[JSC] Add32(Imm, Tmp, Tmp) does not ZDef the destination if Imm is zero
Created attachment 272266 [details] Patch
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 on attachment 272266 [details] Patch r=me
Comment on attachment 272266 [details] Patch Clearing flags on attachment: 272266 Committed r197186: <http://trac.webkit.org/changeset/197186>
All reviewed patches have been landed. Closing bug.
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.
(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.