Proposed change improved some tests performance by allowing to fold Load + Extend to SVE's Load zero/sign extend for fixed vectors.
Details
Diff Detail
Event Timeline
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. |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
5230 |
@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. |
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: