Bug 189939 - lowXYZ in FTLLower should always filter the type of the incoming edge
Summary: lowXYZ in FTLLower should always filter the type of the incoming edge
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-24 17:54 PDT by Saam Barati
Modified: 2018-10-03 20:30 PDT (History)
12 users (show)

See Also:


Attachments
patch (5.84 KB, patch)
2018-09-24 18:05 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (5.23 KB, patch)
2018-09-24 18:06 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (6.27 KB, patch)
2018-09-30 12:09 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2018-09-24 17:54:37 PDT
For example, the FTL may know more about data flow than AI, and it needs to inform AI of certain data flow properties. For example, consider this program:


```
bb#1
a: Phi // Let's say it has an Int32 result, so it goes into the int32 hash table in FTLLower
Branch(...,  #2, #3)

bb#2
ArrayifyToStructure(Cell:@a) // This modifies @a to have some structure
Jump(#3)

bb#3
c: Add(Int32:@something, Int32:@a)
```

When the Add node does lowInt32() for @a, we'll grab it from the int32 hash table. However, we don't update the AI value at that point. But we should. We need to make sure that AI knows that @a is an Int32.

Obviously the above program will OSR exit if it reaches bb#2, however, this is totally legal IR.
Comment 1 Saam Barati 2018-09-24 17:55:21 PDT
<rdar://problem/44407030>
Comment 2 Saam Barati 2018-09-24 18:05:57 PDT
Created attachment 350722 [details]
patch
Comment 3 Saam Barati 2018-09-24 18:06:44 PDT
Created attachment 350723 [details]
patch
Comment 4 Filip Pizlo 2018-09-24 18:27:05 PDT
(In reply to Saam Barati from comment #0)
> For example, the FTL may know more about data flow than AI, and it needs to
> inform AI of certain data flow properties. For example, consider this
> program:
> 
> 
> ```
> bb#1
> a: Phi // Let's say it has an Int32 result, so it goes into the int32 hash
> table in FTLLower
> Branch(...,  #2, #3)
> 
> bb#2
> ArrayifyToStructure(Cell:@a) // This modifies @a to have some structure
> Jump(#3)
> 
> bb#3
> c: Add(Int32:@something, Int32:@a)
> ```
> 
> When the Add node does lowInt32() for @a, we'll grab it from the int32 hash
> table. However, we don't update the AI value at that point. But we should.
> We need to make sure that AI knows that @a is an Int32.

Why?

> 
> Obviously the above program will OSR exit if it reaches bb#2, however, this
> is totally legal IR.
Comment 5 Filip Pizlo 2018-09-24 18:28:37 PDT
Comment on attachment 350723 [details]
patch

This feels super questionable. AI is supposed to be a value add. It’s supposed to be allowed to be imprecise. This patch seems to suggest that someone is relying on AI being maximally precise, which feels like bad form.
Comment 6 Saam Barati 2018-09-24 18:34:49 PDT
(In reply to Filip Pizlo from comment #5)
> Comment on attachment 350723 [details]
> patch
> 
> This feels super questionable. AI is supposed to be a value add. It’s
> supposed to be allowed to be imprecise. This patch seems to suggest that
> someone is relying on AI being maximally precise, which feels like bad form.

The lowXYZ nodes are supposed to give you something that is filtered. They're responsible for doing type checks. So to me, it feels totally correct that they communicate to AI that they did type checks.

This is hitting the assert that says that "you better do an Int32 type check if you are an Int32 use".
Comment 7 Filip Pizlo 2018-09-25 10:41:47 PDT
(In reply to Saam Barati from comment #6)
> (In reply to Filip Pizlo from comment #5)
> > Comment on attachment 350723 [details]
> > patch
> > 
> > This feels super questionable. AI is supposed to be a value add. It’s
> > supposed to be allowed to be imprecise. This patch seems to suggest that
> > someone is relying on AI being maximally precise, which feels like bad form.
> 
> The lowXYZ nodes are supposed to give you something that is filtered.
> They're responsible for doing type checks. So to me, it feels totally
> correct that they communicate to AI that they did type checks.

I don’t follow this logic. The low functions are supposed to give you something of the type that you asked for. That doesn’t mean that if AI is pessimistic then they should make it more precise. 

> 
> This is hitting the assert that says that "you better do an Int32 type check
> if you are an Int32 use".

I disagree with fixing such assertions by making the analysis more precise because AI will never be precise enough to fulfill such assertions. 

Which assertion is this, specifically?  Why not remove it?
Comment 8 Filip Pizlo 2018-09-25 10:48:44 PDT
Additionally, it’s super weird that you are using the backend to increase AI precision. That feels totally backward. And super ineffective since the backend pulls in AI state at each block boundary - so I don’t even see how your fix is comprehensive. Any block boundary would cause that assertion to fire again.

It’s important that this code has a clear story for how it works. It should be: the backend uses AI as a service to get additional optimizations. It should not be: the backend brute forces the AI into saying things that make assertions not fire.
Comment 9 Filip Pizlo 2018-09-25 11:03:33 PDT
I think I'm starting to see the issue.

AI is expecting a type check, but the FTL backend isn't going to do a type check because it magically proved the type using its own tricks - so it didn't need to do a type check.

Therefore, you're emulating the type check that would have happened.

That feels less weird now that I get what's going on.  The basic block thing doesn't seem like a problem either, since the assertion you're fixing is one that has to do with per-node checks.  If there was a block boundary, then it would just mean that we'd do this filtering thing at the first use at the top of the block.
Comment 10 Saam Barati 2018-09-30 11:50:08 PDT
I spoke with Phil offline last week. We think we should use this approach because appeasing the "you better do a type check" AI assertion is valuable. We also think we should make my patch not directly call filter, but call an abstraction over filter, named "simulatedTypeCheck" or something similar.
Comment 11 Saam Barati 2018-09-30 12:09:35 PDT
Created attachment 351217 [details]
patch
Comment 12 Michael Saboff 2018-10-03 16:22:46 PDT
Comment on attachment 351217 [details]
patch

r=me
Comment 13 Saam Barati 2018-10-03 20:03:56 PDT
Comment on attachment 351217 [details]
patch

Thanks for the review.
Comment 14 WebKit Commit Bot 2018-10-03 20:30:05 PDT
Comment on attachment 351217 [details]
patch

Clearing flags on attachment: 351217

Committed r236824: <https://trac.webkit.org/changeset/236824>
Comment 15 WebKit Commit Bot 2018-10-03 20:30:07 PDT
All reviewed patches have been landed.  Closing bug.