Page MenuHomePhabricator

[DAGCombiner] Teach isKnownToBeAPowerOfTwo handle SPLAT_VECTOR.
ClosedPublic

Authored by jacquesguan on Aug 10 2021, 11:47 PM.

Details

Summary

Make DAGCombine turn mul by power of 2 into shl for scalable vector.

Diff Detail

Event Timeline

jacquesguan created this revision.Aug 10 2021, 11:47 PM
jacquesguan requested review of this revision.Aug 10 2021, 11:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2021, 11:47 PM
frasercrmck added inline comments.Aug 11 2021, 9:08 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
783 ↗(On Diff #365674)

I don't think this code is tested by this patch (via fixed-length vectors or VP intrinsics)

DAGCombine should turn mul by 2 into shl. Except that it can't for scalable vectors because isKnownToBeAPowerOfTwo doesn't work for scalable vectors. We should just make DAG combine work even if we have just have to special case SPLAT_VECTOR.

// fold (mul x, (1 << c)) -> x << c                                                                                                                                                                     
if (isConstantOrConstantVector(N1, /*NoOpaques*/ true) &&                                                                                                                                               
    DAG.isKnownToBeAPowerOfTwo(N1) &&                                                                                                                                                                   
    (!VT.isVector() || Level <= AfterLegalizeVectorOps)) {                                                                                                                                              
  SDLoc DL(N);                                                                                                                                                                                          
  SDValue LogBase2 = BuildLogBase2(N1, DL);                                                                                                                                                             
  EVT ShiftVT = getShiftAmountTy(N0.getValueType());                                                                                                                                                    
  SDValue Trunc = DAG.getZExtOrTrunc(LogBase2, DL, ShiftVT);                                                                                                                                            
  return DAG.getNode(ISD::SHL, DL, VT, N0, Trunc);                                                                                                                                                      
}

DAGCombine should turn mul by 2 into shl. Except that it can't for scalable vectors because isKnownToBeAPowerOfTwo doesn't work for scalable vectors. We should just make DAG combine work even if we have just have to special case SPLAT_VECTOR.

Ah nice I missed that, maybe that explains why this patch doesn't test fixed-length vectors, because it's already done. Adding a check for SPLAT_VECTOR seems reasonable to me since it's already got one for BUILD_VECTOR.

Teach isKnownToBeAPowerOfTwo handle SPLAT_VECTOR.

jacquesguan retitled this revision from [RISCV] Select vector mul by 2 to a vector add. to [DAGCombiner] Teach isKnownToBeAPowerOfTwo handle SPLAT_VECTOR..Aug 12 2021, 12:04 AM
jacquesguan edited the summary of this revision. (Show Details)

Now that the scope of this has increased, I think we should try and add scalable-vector tests for all of the DAGCombines which use isKnownToBeAPowerOfTwo.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
3654

This return false isn't doing the right thing and is skipping the computeKnownBits stuff below. I'm surprised the tests are still passing! Are we missing coverage?

I think this should simply be if (C->getAPIntValue().zextOrTrunc(BitWidth).isPowerOf2()) return true, allowing it to fall through to the next checks.

Fix wrong return. Add udiv cases.

Add more cases.

Perhaps some extra tests for muls by other powers of two?

Are you able to get tests for the (mulhu x, (1 << c)) -> x >> (bitwidth - c) combine? In my experience mulhu can be tricky to source. Maybe D49248 contains some hints.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
3650

nit: constant powers -> a constant power

3656

Clang-format's correct; please remove this extra empty line.

The phabricator patch description needs updating.

Fix lint. Add more mul test cases.

Perhaps some extra tests for muls by other powers of two?

Are you able to get tests for the (mulhu x, (1 << c)) -> x >> (bitwidth - c) combine? In my experience mulhu can be tricky to source. Maybe D49248 contains some hints.

mulhu cases with constant splat can not be generated from combineShiftToMULH now, I create a new diff D108129 to handle it.

frasercrmck accepted this revision.Aug 17 2021, 10:12 AM

LGTM, thanks!

This revision is now accepted and ready to land.Aug 17 2021, 10:12 AM