This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add vector integer mul/mulh/div/rem ISel patterns
ClosedPublic

Authored by frasercrmck on Jan 5 2021, 2:29 AM.

Details

Summary

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.

Diff Detail

Event Timeline

frasercrmck created this revision.Jan 5 2021, 2:29 AM
frasercrmck requested review of this revision.Jan 5 2021, 2:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2021, 2:29 AM

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.

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

This revision is now accepted and ready to land.Jan 5 2021, 2:07 PM
  • rebase and reenable urem tests

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.

frasercrmck edited the summary of this revision. (Show Details)Jan 6 2021, 12:47 AM