[PATCH] D158853: [SDAG] Add SimplifyDemandedBits support for ISD::SPLAT_VECTOR_PARTS

[Luke Lau via Phabricator via llvm-commits]

luke added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:1168
+  case ISD::SPLAT_VECTOR_PARTS: {
+    unsigned NumSclBits = Op.getOperand(0).getScalarValueSizeInBits();
+    assert(NumSclBits * Op.getNumOperands() == BitWidth &&
----------------
reames wrote:
> I think Scl is short for Scalar?  If so, just write it.  Scl isn't a common abbreviation.  
Yeah I had never seen this abbreviation before, but it's what's used in the rest of this function so I just chose it for consistency. Happy to expand it if preferred, I'm not strongly opinionated.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vnsrl-sdnode.ll:644
   %splat = shufflevector <vscale x 1 x i64> %head, <vscale x 1 x i64> poison, <vscale x 1 x i32> zeroinitializer
   %vb = trunc <vscale x 1 x i64> %splat to <vscale x 1 x i32>
   %x = lshr <vscale x 1 x i32> %va, %vb
----------------
reames wrote:
> craig.topper wrote:
> > luke wrote:
> > > craig.topper wrote:
> > > > craig.topper wrote:
> > > > > This looks like what we really have is a missing combine on trunc of splat_vector.
> > > > Not objecting to this patch just that it might not show the proper motivation.
> > > Is the combine you're referring to something like:
> > > 
> > > ```
> > >               t4: i32,ch = CopyFromReg t0, Register:i32 %1
> > >               t6: i32,ch = CopyFromReg t0, Register:i32 %2
> > >             t8: i64 = build_pair t4, t6
> > >           t11: nxv1i64 = splat_vector t8
> > >         t13: nxv1i32 = truncate t11
> > > ```
> > > 
> > > to
> > > 
> > > ```
> > >               t4: i32,ch = CopyFromReg t0, Register:i32 %1
> > >         t13: nxv1i32 = splat_vector t4
> > > ```
> > > 
> > > And not on splat_vector_parts after legalisation?
> > Do we already have a combine for truncate of build_pair?
> > 
> > From a quick glance, I see that we do convert (truncate (build_vector X, Y, Z)) to (build_vector (trunc X), (trunc Y), (trunc Z)). here.
> > 
> > ```
> >   // Attempt to pre-truncate BUILD_VECTOR sources.                               
> >   if (N0.getOpcode() == ISD::BUILD_VECTOR && !LegalOperations &&                 
> >       TLI.isTruncateFree(SrcVT.getScalarType(), VT.getScalarType()) &&           
> >       // Avoid creating illegal types if running after type legalizer.           
> >       (!LegalTypes || TLI.isTypeLegal(VT.getScalarType()))) {                    
> >     SDLoc DL(N);                                                                 
> >     EVT SVT = VT.getScalarType();                                                
> >     SmallVector<SDValue, 8> TruncOps;                                            
> >     for (const SDValue &Op : N0->op_values()) {                                  
> >       SDValue TruncOp = DAG.getNode(ISD::TRUNCATE, DL, SVT, Op);                 
> >       TruncOps.push_back(TruncOp);                                               
> >     }                                                                            
> >     return DAG.getBuildVector(VT, DL, TruncOps);                                 
> >   }
> > ```
> > 
> > Maybe we should do the same for splat_vector?
> I agree with Craig here. We definitely should have a trunc(splat) to splat(trunc) transform.  If we don't already, we should fix that.  
> 
> To be clear, this patch can be landed.  You need to add a bit more test coverage for cases which *aren't* truncates so that once the other patch lands we still have test coverage for this code, but that's about the only thing missing.
> 
> Or said differently, Craig and I are suggesting *additional* work, not *alternative* work.  
I'm in agreement too here, happy to submit a patch for that. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158853/new/

https://reviews.llvm.org/D158853