[PATCH] D37069: [x86] use the IR type of formal args to create assertzext/assertsext and scalar truncate nodes

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8734
 
         ArgValues.push_back(getCopyFromParts(DAG, dl, &InVals[i], NumParts,
                                              PartVT, VT, nullptr, AssertOp,
----------------
aaboud wrote:
> spatel wrote:
> > aaboud wrote:
> > > Do we still need this?
> > > It is the reason for the second AssertZext instruction.
> > I don't think we'll get the 2nd assert for the affected tests with this patch because we trunc to i1, but let me know if you're seeing something else.
> > 
> > But yes, we do need this code - or at least there's some other change needed to account for that part. If I just don't set AssertOp, we'll get these x86 test fails and from what I saw it's because we need extra masking ops without the assert:
> > Failing Tests (5):
> >     LLVM :: CodeGen/X86/bool-zext.ll
> >     LLVM :: CodeGen/X86/illegal-bitfield-loadstore.ll
> >     LLVM :: CodeGen/X86/negate-i1.ll
> >     LLVM :: CodeGen/X86/sext-i1.ll
> >     LLVM :: CodeGen/X86/tail-call-casts.ll
> > 
> I was using this test to debug the second AssertSext:
> 
> 
> ```
> target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
> 
> 
> define i32  @sbar(i1 signext %a) {
>  %b = xor i1 %a, 1
>  %sext = sext i1 %b to i32
>  ret i32 %sext
> }
> ```
> 
> command line:
> llc -mtriple=x86_64-unknown-unknown -o - 
> 
> DAG after this line:
> 
> ```
> SelectionDAG has 9 nodes:
>             t0: ch = EntryToken
>           t2: i32,ch = CopyFromReg t0, Register:i32 %vreg0
>         t4: i32 = AssertSext t2, ValueType:ch:i8
>       t5: i8 = truncate t4
>     t7: i8 = AssertSext t5, ValueType:ch:i1
>   t8: i1 = truncate t7
>   t0: ch = EntryToken
> ```
Yes - that's exactly the case that this patch should be hitting. I see:
Creating new node: t4: i32 = AssertSext t2, ValueType:ch:i1
Creating new node: t5: i1 = truncate t4
...so there's no extra trunc+assertzext with this patch. You're not seeing that?


https://reviews.llvm.org/D37069