Bug 154704

Summary: [JSC] Add32(Imm, Tmp, Tmp) does not ZDef the destination if Imm is zero
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, keith_miller, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Benjamin Poulain 2016-02-25 16:54:58 PST
[JSC] Add32(Imm, Tmp, Tmp) does not ZDef the destination if Imm is zero
Comment 1 Benjamin Poulain 2016-02-25 17:17:46 PST
Created attachment 272266 [details]
Patch
Comment 2 WebKit Commit Bot 2016-02-25 17:19:04 PST
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.
Comment 3 Geoffrey Garen 2016-02-26 00:15:00 PST
Comment on attachment 272266 [details]
Patch

r=me
Comment 4 WebKit Commit Bot 2016-02-26 11:18:50 PST
Comment on attachment 272266 [details]
Patch

Clearing flags on attachment: 272266

Committed r197186: <http://trac.webkit.org/changeset/197186>
Comment 5 WebKit Commit Bot 2016-02-26 11:18:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Filip Pizlo 2016-02-27 15:09:51 PST
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.
Comment 7 Filip Pizlo 2016-02-27 15:10:27 PST
(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.