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

[Amjad Aboud via Phabricator via llvm-commits]

aaboud added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8734
 
         ArgValues.push_back(getCopyFromParts(DAG, dl, &InVals[i], NumParts,
                                              PartVT, VT, nullptr, AssertOp,
----------------
spatel wrote:
> 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?
You are right, this patch prevent this code from generating another AssertSext.
Now we return from getCopyFromParts at line 222, before adding the new AsserSext.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:3009
         // Handle MMX values passed in XMM regs.
-        if (RegVT.isVector() && VA.getValVT().getScalarType() != MVT::i1)
+        if (RegVT.isVector() && VA.getValVT().getScalarType() != MVT::i1) {
           ArgValue = DAG.getNode(X86ISD::MOVDQ2Q, dl, VA.getValVT(), ArgValue);
----------------
by the way, do we need these parentheses here and below with the one line "else if"/"else" ?


https://reviews.llvm.org/D37069