Bug 153213 – Fix Air shuffling assertions

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Running JSC tests in debug shows a lot of assertions in shuffling.  Patch forthcoming.

[Filip Pizlo]

Created attachment 269243 [details]
fixed two bugs so far

[Filip Pizlo]

Created attachment 269244 [details]
the patch

[WebKit Commit Bot]

Attachment 269244 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/testair.cpp:716:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/testair.cpp:717:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/testair.cpp:718:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/testair.cpp:719:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/testair.cpp:720:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/testair.cpp:721:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
Total errors found: 6 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 269244 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269244&action=review

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:-736
> -        ASSERT(-128 <= imm.m_value && imm.m_value < 128);

Why get rid of this assertion?

[Filip Pizlo]

Comment on attachment 269244 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269244&action=review

>> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:-736
>> -        ASSERT(-128 <= imm.m_value && imm.m_value < 128);
> 
> Why get rid of this assertion?

It was firing. 

Air sometimes does Store8 with a 8-bit immediate that isn't in this range. For example "250". It obviously makes sense as an immediate for Store8, but it fails this assertion. 

I don't think it makes sense to teach Air to turn immediately passed to Store8 into the form that would placate this assertion. I cannot imagine what bug this assertion could ever catch. But I can imagine that I'd have to significantly change how Air handles constants to be able to preserve this assertion.

[Saam Barati]

Comment on attachment 269244 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269244&action=review

>>> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:-736
>>> -        ASSERT(-128 <= imm.m_value && imm.m_value < 128);
>> 
>> Why get rid of this assertion?
> 
> It was firing. 
> 
> Air sometimes does Store8 with a 8-bit immediate that isn't in this range. For example "250". It obviously makes sense as an immediate for Store8, but it fails this assertion. 
> 
> I don't think it makes sense to teach Air to turn immediately passed to Store8 into the form that would placate this assertion. I cannot imagine what bug this assertion could ever catch. But I can imagine that I'd have to significantly change how Air handles constants to be able to preserve this assertion.

You could assert that the upper 24 bits are zero if passed in a number with absolute value
>= 128

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:-737
> -        m_assembler.movb_i8m(imm.m_value, address.offset, address.base);

I agree though that this probably isn't finding any bugs in our code.

[Filip Pizlo]

(In reply to comment #6)
> Comment on attachment 269244 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269244&action=review
> 
> >>> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:-736
> >>> -        ASSERT(-128 <= imm.m_value && imm.m_value < 128);
> >> 
> >> Why get rid of this assertion?
> > 
> > It was firing. 
> > 
> > Air sometimes does Store8 with a 8-bit immediate that isn't in this range. For example "250". It obviously makes sense as an immediate for Store8, but it fails this assertion. 
> > 
> > I don't think it makes sense to teach Air to turn immediately passed to Store8 into the form that would placate this assertion. I cannot imagine what bug this assertion could ever catch. But I can imagine that I'd have to significantly change how Air handles constants to be able to preserve this assertion.
> 
> You could assert that the upper 24 bits are zero if passed in a number with
> absolute value
> >= 128

Imagine I have a Store8 like this:

Move 0x1234, %tmp
...
Store8 %tmp, (%ptr)

Then let's say that the spiller spills:

Move 0x123, (stack)
...
Move (stack), %tmp'
Store8 %tmp', (%ptr)

Then we want the rematerialization to realize that we're talking about an immediate and constant-propagate through the spill and get this:

Store8 $1234, (%ptr)

Why did the rematerialization know that it could do this?  Other than the forward flow to prove the value of %tmp', it knew that a "Arg::imm" is acceptable to Store8 and on x86 Arg::imm accepts any 32-bit immediate. So, the remat code isn't going to the special case for Store8 and clip the immediate just to placate Storr8's assertion. We won't do that because the whole point of operands in Air is that you can reason about what is valid just based on high-level queries (does arg#0 of store8 admit an immediate and does an immediate admit 0x1234). 

To make any version of this assertion work, we'd have to teach Air that x86 has an additional kind of immediate, Arg::imm8. Then we would teach Store8 that it doesn't really admit imm but imm8. We'd have to teach all other instructions that admit imm that this implies that they also admit imm8. We'd have to teach all phases that deal with immediates that there is a new immediate. 

That's a huge nightmare. The lesser evil is to just say that Store8 admits a 32-bit immediate just like the rest of the instructions do, and then make sure that Store8 ignores the upper 24 bits of the immediate. 

> 
> > Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:-737
> > -        m_assembler.movb_i8m(imm.m_value, address.offset, address.base);
> 
> I agree though that this probably isn't finding any bugs in our code.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/195296

Follow-up fix landed in http://trac.webkit.org/changeset/195297