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.