This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][CodeGen][SME] Allow vectors large than hardware support to use SVE's load zero/sign-extend for fixed vectors
ClosedPublic

Authored by dtemirbulatov on Apr 4 2023, 7:00 AM.

Details

Summary

Proposed change improved some tests performance by allowing to fold Load + Extend to SVE's Load zero/sign extend for fixed vectors.

Diff Detail

Event Timeline

dtemirbulatov created this revision.Apr 4 2023, 7:00 AM
dtemirbulatov requested review of this revision.Apr 4 2023, 7:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2023, 7:00 AM
david-arm added inline comments.Apr 4 2023, 7:34 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
5230

Hi @dtemirbulatov, I personally feel this patch should be only trying to fix one thing at a time, but it seems to be trying to change both SVE fixed-length behaviour as well as streaming SVE behaviour. Perhaps it's better to focus on just the streaming SVE case for now, in which case I think you can more simply write this as:

bool OverrideNEON = Subtarget->useSVEForFixedLengthVectors() || Subtarget->forceStreamingCompatibleSVE();
return ExtVal.getValueType().isScalableVector() ||
       useSVEForFixedLengthVectorVT(
           ExtVal.getValueType(), OverrideNEON);
5233

Just for reference, I'm not sure if this is really what you want for the SVE fixed-length case (no streaming SVE) because getMinSVEVectorSizeInBits() could return 256 or greater. In this case it has nothing to do with NEON-sized vectors, which are 64-bit or 128-bit. In any case, I feel like the normal SVE fixed-length case should be done in a different patch.

llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-int-mulh.ll
171 ↗(On Diff #510791)

Hi @dtemirbulatov, this looks like a regression to me. We've gone from 20 instructions to 29, which doesn't seem right. Can you investigate why this is happening?

248 ↗(On Diff #510791)

Again, this has gotten worse - from 20 -> 29 instructions.

Matt added a subscriber: Matt.Apr 5 2023, 10:37 PM
sdesmalen added inline comments.Apr 13 2023, 4:07 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
5230
bool OverrideNEON = Subtarget->useSVEForFixedLengthVectors() || Subtarget->forceStreamingCompatibleSVE();

@david-arm just pointing out that if forceStreamingCompatibleSVE() is true, then useSVEForFixedLengthVectors() is also true, so your suggestion is actually equivalent to the original code.

5230–5232

When I change this code to:

return ExtVal.getValueType().isScalableVector() || Subtarget->useSVEForFixedLengthVectors();

all the tests still pass. That suggests that the test for 'ExtVal.getSizeInBits() > getMinSVEVectorSizeInBits()' is not used.

Rebasing after rGe6096871fdd4 commit.

This revision is now accepted and ready to land.Apr 19 2023, 6:47 AM
dtemirbulatov marked 2 inline comments as done.Apr 19 2023, 6:49 AM
This revision was landed with ongoing or failed builds.Apr 19 2023, 7:18 AM
This revision was automatically updated to reflect the committed changes.