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

[Philip Reames via Phabricator via llvm-commits]

reames 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 &&
----------------
I think Scl is short for Scalar?  If so, just write it.  Scl isn't a common abbreviation.  


================
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
----------------
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.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158853