This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen][SVE] Calculate correct type legalization for scalable vectors.
ClosedPublic

Authored by sdesmalen on May 27 2020, 8:50 AM.

Details

Summary

This patch updates TargetLoweringBase::computeRegisterProperties and
TargetLoweringBase::getTypeConversion to support scalable vectors,
and make the right calls on how to legalise them. These changes are required
to legalise both MVTs and EVTs.

Diff Detail

Event Timeline

sdesmalen created this revision.May 27 2020, 8:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2020, 8:50 AM
efriedma added inline comments.May 27 2020, 4:08 PM
llvm/include/llvm/CodeGen/TargetLowering.h
189

getTypeAction() and getTypeToTransformTo() are public; the unittest can use those APIs.

llvm/lib/CodeGen/TargetLoweringBase.cpp
942

report_fatal_error, and please move this up to just after the NumElts.isOne() check. (The EltVT.isInteger() code also assumes the element count can be divided by two.)

1350

We probably want a new LegalizeTypeAction kind to represent this. I guess just leaving it TypeLegal isn't too terrible for now, though.

david-arm added inline comments.May 28 2020, 1:09 AM
llvm/include/llvm/Support/TypeSize.h
54

Alternatively here we could have a more powerful isEqual() where we pass an unsigned integer of the same type in ElementCount, then you can simply do:

if (EC.isEqual(1)) { ... }

The function would look like

bool isEqual(const unsigned RHS) const {
  return Min == RHS && !Scalable;
}

If you don't want to do that in this patch that's fine, but it might be more generic?

llvm/lib/CodeGen/TargetLoweringBase.cpp
826

Instead of calling EVT::getVectorVT, could we just call SVT.getHalfNumVectorElementsVT() instead? It does the same thing.

sdesmalen updated this revision to Diff 266782.May 28 2020, 2:28 AM
  • Use public interfaces in tests (getTypeAction and getTypeToTransformTo)
  • Changed llvm_unreachable -> report_fatal_error
  • Added operator==(unsigned RHS) to ElementCount in favour of isOne()
sdesmalen marked 6 inline comments as done.May 28 2020, 2:32 AM
sdesmalen added inline comments.
llvm/lib/CodeGen/TargetLoweringBase.cpp
1350

Something like TypeCannotBeLegalized?

efriedma added inline comments.May 28 2020, 1:30 PM
llvm/lib/CodeGen/TargetLoweringBase.cpp
1350

I was thinking something more like "TypeScalarizeVScaleVector". It's theoretically possible to legalize... although it's complicated by the fact that we can't easily emit loops in SelectionDAG.

sdesmalen updated this revision to Diff 268234.Jun 3 2020, 9:28 AM
sdesmalen marked an inline comment as done.

Added TypeScalarizeScalableVector enum value.

sdesmalen marked an inline comment as done.Jun 3 2020, 9:28 AM
efriedma accepted this revision.Jun 4 2020, 4:03 PM

LGTM

This revision is now accepted and ready to land.Jun 4 2020, 4:03 PM
This revision was automatically updated to reflect the committed changes.
foad added a subscriber: foad.Jun 5 2020, 8:26 AM

@sdesmalen this is causing test failures in my DEBUG build:

Failing Tests (4):
  LLVM :: CodeGen/AMDGPU/fmax_legacy.f16.ll
  LLVM :: CodeGen/AMDGPU/fmin_legacy.f16.ll
  LLVM :: CodeGen/AMDGPU/select-vectors.ll
  LLVM :: CodeGen/AMDGPU/select.f16.ll

@sdesmalen this is causing test failures in my DEBUG build:

Failing Tests (4):
  LLVM :: CodeGen/AMDGPU/fmax_legacy.f16.ll
  LLVM :: CodeGen/AMDGPU/fmin_legacy.f16.ll
  LLVM :: CodeGen/AMDGPU/select-vectors.ll
  LLVM :: CodeGen/AMDGPU/select.f16.ll

Yes sorry for that. I had already reverted the patch and am looking into it. Cheers for pointing out!