[X86] Fix pattern match for 32-to-64-bit zext in the presence of AssertSext

[mkuper]

Summary

This fixes PR20494 ( http://llvm.org/bugs/show_bug.cgi?id=20494 ).
I'm not at all sure this is the right way to fix it, other ideas are welcome.

[mkuper]

Uploaded diff with proper context, sorry about the first one.

[spatel]

Michael,
Thanks for working on this. If your patch doesn't cause any existing regression tests to fail, then you got further than I did. :)
It seems like a good solution to me, but let's have someone more familiar with the x86 backend take a look...or any backend? You said this bug also affects AArch64 and MSP430?

[mkuper]

AArch64 and MSP have very similar patterns in their TD files - I guess copied from x86 - so I suspect they have the same issue.
I don't know if they actually have the same bug, and I don't have either the hardware or the familiarity with their ISA to patch them, even though the actual patch ought to look the same.

Anyway, if you have any potential reviewers in mind (other than Nadav who's already on the review), feel free to suggest.

[nadav]

Michael,

Is there a way to reduce the size of the testcase? Do you need two basic blocks? Can you add additional check lines to make sure that the ‘mov’ that you are matching is the one you are expecting?

Thanks,
Nadav

[mkuper]

Yes, you need two basic blocks - the bug happens when you have a CopyFromReg -> AssertSext -> Zext chain, so trivially merging the basic blocks would make the issue go away.
Is there a simpler way to generate a CopyFromReg?

Regarding the mov - the resulting assembly is basically this:

orq $-2, %rcx
movl %ecx, %eax
imulq $7, %rax
shrq $32, %rax
retq

I can match the dest of the movl going into the imulq.
Would that be better?

[mkuper]

On second thought, I could just get rid of most things after the zext - they were good for the reproducer, but probably not needed for the testcase.
The or before the first trunc is a must as that's what causes the AssertSext to be valid.
I'll try to reduce the testcase and upload a new patch.

[qcolombet]

A quick comment:
Please do not use date in the filename for the test case.

Thanks,
-Quentin

[mkuper]

Shortened the testcase and changed the name.
I think checking for a mov with the specific registers is the right thing to do, as the registers are determined by the ABI - the function parameter must come in ecx, and the ret value must go to eax.
Nadav, if you think that's too specific, let me know.

Michael

[nadav]

Can you please add a check line for ‘ret’ ? I want to make sure that the check line does not mix with code from the following test. With that the code LGTM.

[mkuper]

Closed by commit rL221626 (authored by @mkuper).

llvm/trunk/lib/Target/X86/X86InstrCompiler.td

 // register. Truncate can be lowered to EXTRACT_SUBREG. CopyFromReg may
 // be copying from a truncate. And x86's cmov doesn't do anything if the
 // condition is false. But any other 32-bit operation will zero-extend
 // up to 64 bits.
 def def32 : PatLeaf<(i32 GR32:$src), [{
   return N->getOpcode() != ISD::TRUNCATE &&
          N->getOpcode() != TargetOpcode::EXTRACT_SUBREG &&
          N->getOpcode() != ISD::CopyFromReg &&
+         N->getOpcode() != ISD::AssertSext &&
          N->getOpcode() != X86ISD::CMOV;
 }]>;
 
 // In the case of a 32-bit def that is known to implicitly zero-extend,
 // we can use a SUBREG_TO_REG.
 def : Pat<(i64 (zext def32:$src)),
           (SUBREG_TO_REG (i64 0), GR32:$src, sub_32bit)>;
 

llvm/trunk/test/CodeGen/X86/TruncAssertZext.ll

+; RUN: llc < %s -O2 -march=x86-64 | FileCheck %s
+; Checks that a zeroing mov is inserted for the trunc/zext pair even when
+; the source of the zext is an AssertSext node
+; PR20494
+
+define i64 @main(i64 %a) { 
+; CHECK-LABEL: main
+; CHECK: movl %ecx, %eax
+; CHECK: ret
+  %or = or i64 %a, -2
+  %trunc = trunc i64 %or to i32
+  br label %l
+l:
+  %ext = zext i32 %trunc to i64
+  ret i64 %ext
+}