Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM with one minor
llvm/lib/CodeGen/ScalarizeMaskedMemIntrin.cpp | ||
---|---|---|
522–523 | I know you're being cautious about changes but I think this assertion can use isa<FixedVectorType>? Or drop the assert entirely - cast<> implicitly asserts for the correct type. |
Thanks for checking @ctetreau. I actually think most of the changes are fine to go in now, with the exception of:
- the cast to FixedVectorType in llvm/lib/CodeGen/ValueTypes.cpp.
- the changes to InterleavedAccessPass, because this pass is not yet guarded by bailing out early.
The other changes are fine because:
- The ACLE uses custom target-specific intrinsics instead of llvm.masked.load/store/gather/scatter intrinsics.
- The ACLE uses custom target-specific intrinsics instead of the llvm.experimental.reduction intrinsics.
- GlobalISel already falls back to DAGISel if the function uses scalable vectors (D81557 and D82524)
- The InterleavedLoadCombinePass was guarded in D79700 to bail out early for scalable vectors.
- shouldConvertSplatType returns nullptr for AArch64 so the change in CodeGenPrepare::optimizeShuffleVectorInst is never hit.
If you can add a check to bail out early for the InterleavedAccessPass for scalable vectors and move the change to EVT::getExtendedVectorNumElements into a separate patch, then I'm happy with most of this to go in now.
@sdesmalen I went ahead and made the requested changes. In ValueType.cpp, rather than leaving the code alone, I went ahead and returned getElementCount().Min along with adding a warning if the vector was scalable. This way getNumElement() can be removed in the future without having to touch this file again.
I know you're being cautious about changes but I think this assertion can use isa<FixedVectorType>? Or drop the assert entirely - cast<> implicitly asserts for the correct type.