Bug 189939 – lowXYZ in FTLLower should always filter the type of the incoming edge

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

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.

[Saam Barati]

<rdar://problem/44407030>

[Saam Barati]

Created attachment 350722 [details]
patch

[Saam Barati]

Created attachment 350723 [details]
patch

[Filip Pizlo]

(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.

[Filip Pizlo]

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.

[Saam Barati]

(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".

[Filip Pizlo]

(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?

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Saam Barati]

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.

[Saam Barati]

Created attachment 351217 [details]
patch

[Michael Saboff]

Comment on attachment 351217 [details]
patch

r=me

[Saam Barati]

Comment on attachment 351217 [details]
patch

Thanks for the review.

[WebKit Commit Bot]

Comment on attachment 351217 [details]
patch

Clearing flags on attachment: 351217

Committed r236824: <https://trac.webkit.org/changeset/236824>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.