Bug 154704 – [JSC] Add32(Imm, Tmp, Tmp) does not ZDef the destination if Imm is zero

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[JSC] Add32(Imm, Tmp, Tmp) does not ZDef the destination if Imm is zero

[Benjamin Poulain]

Created attachment 272266 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/b3/testb3.cpp:431:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
Total errors found: 1 in 3 files


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

[Geoffrey Garen]

Comment on attachment 272266 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 272266 [details]
Patch

Clearing flags on attachment: 272266

Committed r197186: <http://trac.webkit.org/changeset/197186>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

Comment on attachment 272266 [details]
Patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:204
>      void add32(TrustedImm32 imm, RegisterID src, RegisterID dest)
>      {
>          if (!imm.m_value) {
> -            move(src, dest);
> +            zeroExtend32ToPtr(src, dest);
>              return;
>          }
>  

I think that this is still wrong.  Can we please remove this strength reduction rule from the MacroAssembler?  Unless a move instruction sets the same flags as an add32 instruction, then this constant-folding rule will cause us great confusion if we ever start reasoning about flags.  Reasoning about flags unlocks smart things, like realizing that if you compare the result of an addition to zero, you can usually just remove the compare (or test) instruction and branch directly on add's flags.

I've already removed all other places that I found where we did this !imm.m_value optimization, because they were also causing ZDef bugs.  I then removed the add32AndSetFlags() thing because that appeared to mostly be for avoiding this optimization.  So, now the expectation is that add32() sets flags.

[Filip Pizlo]

(In reply to comment #6)
> Comment on attachment 272266 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272266&action=review
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:204
> >      void add32(TrustedImm32 imm, RegisterID src, RegisterID dest)
> >      {
> >          if (!imm.m_value) {
> > -            move(src, dest);
> > +            zeroExtend32ToPtr(src, dest);
> >              return;
> >          }
> >  
> 
> I think that this is still wrong.  Can we please remove this strength
> reduction rule from the MacroAssembler?  Unless a move instruction sets the
> same flags as an add32 instruction, then this constant-folding rule will
> cause us great confusion if we ever start reasoning about flags.  Reasoning
> about flags unlocks smart things, like realizing that if you compare the
> result of an addition to zero, you can usually just remove the compare (or
> test) instruction and branch directly on add's flags.
> 
> I've already removed all other places that I found where we did this
> !imm.m_value optimization, because they were also causing ZDef bugs.  I then
> removed the add32AndSetFlags() thing because that appeared to mostly be for
> avoiding this optimization.  So, now the expectation is that add32() sets
> flags.

I just realized that I'm wrong - three-operand add32() on X86 doesn't set flags!

Never mind everything I said above.