There is no test coverage for the mulhs or mulhu patterns as I can't get
the DAGCombiner to generate them for scalable vectors. There are a few
places in that still need updating for that to work. I left the patterns
in regardless as they are correct.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
For a bit more context around the (surprising) createConstant issue:
// In other cases the element type is illegal and needs to be expanded, for
// example v2i64 on MIPS32. In this case, find the nearest legal type, split
// the value into n parts and use a vector type with n-times the elements.
// Then bitcast to the type requested.
// Legalizing constants too early makes the DAGCombiner's job harder so we
// only legalize if the DAG tells us we must produce legal types.
else if (NewNodesMustHaveLegalTypes && VT.isVector() &&
TLI->getTypeAction(*getContext(), EltVT) ==
TargetLowering::TypeExpandInteger) {
const APInt &NewVal = Elt->getValue();
EVT ViaEltVT = TLI->getTypeToTransformTo(*getContext(), EltVT);
unsigned ViaEltSizeInBits = ViaEltVT.getSizeInBits();
unsigned ViaVecNumElts = VT.getSizeInBits() / ViaEltSizeInBits;
EVT ViaVecVT = EVT::getVectorVT(*getContext(), ViaEltVT, ViaVecNumElts);
// Check the temporary vector is the correct size. If this fails then
// getTypeToTransformTo() probably returned a type whose size (in bits)
// isn't a power-of-2 factor of the requested type size.
assert(ViaVecVT.getSizeInBits() == VT.getSizeInBits());It should be obvious to see how this has no chance of working for scalable vectors.
I don't fully understand the constraints on NewNodesMustHaveLegalTypes: is it permissible to ask the target to custom-lower this case if they've marked SPLAT_VECTOR as Custom? Obviously we'd like this to be lowered to our SPLAT_VECTOR_I64 if possible and to our custom sequence otherwise.
I'm not sure if we can call the custom handlers from inside of createConstant. We're technically in DAG combine not a legalization step. Why is DAGCombiner calling createConstant?
I think this specific issue on REM should be fixed after 4ef91f5871a3c38bb2324f89b47a2a845e8a33fd
Thanks, Craig. I mulled over doing that same thing. Hopefully it buys us enough time to fix the issue properly.
Yeah, I thought custom-lowering was a push. I can also see how easy it'd be to get stuck in a loop if you called any other methods when creating this constant. I was thinking you could maybe support this with bitcasts and shifts but it'd be expensive. I might bring this up in the SVE call and see if anyone has ideas.