Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[SDAG] Add SimplifyDemandedBits support for ISD::SPLAT_VECTOR_PARTS
Needs ReviewPublic

Authored by luke on Aug 25 2023, 8:30 AM.



Similar to, this allows some shift and rotate
operations on RV32 to better select an immediate or scalar operand, due to the
upper bits of the splat being marked as undef.

Diff Detail

Event Timeline

luke created this revision.Aug 25 2023, 8:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 8:31 AM
luke requested review of this revision.Aug 25 2023, 8:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 8:31 AM
craig.topper added inline comments.Aug 25 2023, 10:27 AM
644 ↗(On Diff #553495)

This looks like what we really have is a missing combine on trunc of splat_vector.

644 ↗(On Diff #553495)

Not objecting to this patch just that it might not show the proper motivation.

luke added inline comments.Aug 29 2023, 7:35 AM
644 ↗(On Diff #553495)

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


      t4: i32,ch = CopyFromReg t0, Register:i32 %1
t13: nxv1i32 = splat_vector t4

And not on splat_vector_parts after legalisation?

craig.topper added inline comments.Aug 29 2023, 9:21 AM
644 ↗(On Diff #553495)

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);                 
  return DAG.getBuildVector(VT, DL, TruncOps);                                 

Maybe we should do the same for splat_vector?

reames added inline comments.Aug 29 2023, 11:47 AM

I think Scl is short for Scalar? If so, just write it. Scl isn't a common abbreviation.

644 ↗(On Diff #553495)

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.

luke added inline comments.Aug 29 2023, 3:46 PM

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.

644 ↗(On Diff #553495)

I'm in agreement too here, happy to submit a patch for that.

luke updated this revision to Diff 555119.Aug 31 2023, 11:31 AM

Rebase on top of new test cases

luke added inline comments.Aug 31 2023, 11:34 AM

This a0 is actually undef, it's just selected by coincidence because there's no combine after RISCVDAGToDAGISel::PreprocessISelDAG to remove the store of undef. Might it be worthwhile short-circuiting a store of undef directly in SelectionDAG::getStore?