Page MenuHomePhabricator

[AArch64][CodeGen] Always use SVE (when enabled) to lower integer divides
ClosedPublic

Authored by david-arm on Jan 21 2022, 3:56 AM.

Details

Summary

This patch adds custom lowering support for ISD::SDIV and ISD::UDIV
when SVE is enabled, regardless of the minimum SVE vector length. We do
this because NEON simply does not have vector integer divide support, so
we want to take advantage of these instructions in SVE.

As part of this patch I've also simplified LowerToPredicatedOp to avoid
re-asking the same question about whether we should be using SVE for
fixed length vectors. Once we've made the decision to call
LowerToPredicatedOp, then we should simply assert we should be using SVE.

I've pulled the 128-bit min SVE vector bits tests out of

CodeGen/AArch64/sve-fixed-length-int-div.ll
CodeGen/AArch64/sve-fixed-length-int-rem.ll

and moved them here instead

CodeGen/AArch64/sve-fixed-length-int-div-128.ll
CodeGen/AArch64/sve-fixed-length-int-rem-128.ll

Diff Detail

Event Timeline

david-arm created this revision.Jan 21 2022, 3:56 AM
david-arm requested review of this revision.Jan 21 2022, 3:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2022, 3:56 AM
david-arm edited the summary of this revision. (Show Details)Jan 21 2022, 4:05 AM
paulwalker-arm added inline comments.Jan 21 2022, 4:42 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11175

Here I think VT.isFixedLengthVector() && hasSVE() is all you need. This is operation legalisation so the types must already be legal. The current useSVEForFixedLengthVectorVT call exists purely to prevent the one case you no longer want to prevent.

llvm/lib/Target/AArch64/AArch64ISelLowering.h
497–501

My immediate reaction is that I'm not sure this enum is required. It exists so that you can pass Always to useSVEForFixedLengthVectorVT but under those circumstances I don't believe there's a need to ever call useSVEForFixedLengthVectorVT.

Matt added a subscriber: Matt.Jan 25 2022, 3:16 PM
david-arm updated this revision to Diff 404533.Jan 31 2022, 7:52 AM
david-arm edited the summary of this revision. (Show Details)
  • Removed the NeonOverride enum from previous patch.
david-arm marked 2 inline comments as done.Jan 31 2022, 7:52 AM
sdesmalen added inline comments.Jan 31 2022, 8:11 AM
llvm/test/CodeGen/AArch64/sve-fixed-length-int-div.ll
1

Instead of duplicating these tests and moving these check lines to a different file, I'd rather have a RUN line with a check-prefix=VBITS_EQ_128, where you only add check lines for the cases where it currently already has CHECK and CHECK-NEXT lines.

For all places where the expected CHECK lines are different from the expected output for VBITS_EQ_128, please add a FIXME: The code should be equivalent. Remove CHECK lines in favour of VBITS_EQ_128. or something like that. When we fix the issue that having a larger vector width generates different code for the same fixed-width vectors, it will be obvious which lines to remove, and we don't end up with a stale test file (e.g. sve-fixed-length-int-div-128.ll)

david-arm updated this revision to Diff 404937.Feb 1 2022, 7:50 AM
  • Moved the 128-bit tests back into the original file.
david-arm added inline comments.Feb 1 2022, 7:52 AM
llvm/test/CodeGen/AArch64/sve-fixed-length-int-rem.ll
696
NOTE: The codegen here would be improved if we also used SVE for 64-bit element vector multiplies!
paulwalker-arm accepted this revision.Feb 1 2022, 4:53 PM

The code looks fine to me. For the tests I would prefer it if you removed the new VBITS_EQ_128 lines for the >128bit vector tests as I don't believe they're testing anything new that isn't already being tested by the 64 and 128 bit vector tests. More than that, because you're hardwiring all the output I can see it just being a source of pain as the tests are more likely to need to be manually updated after unrelated code generation changes, which is something that has previously annoyed people. If you think the checks offer real value then as a minimum please consider at least removing them from the v32i8 variants as they're likely to be the worst offenders.

llvm/test/CodeGen/AArch64/sve-fixed-length-int-div.ll
186

Do you need these CHECK lines? I would have thought it's only worth VBITS_EQ_128 CHECk lines for the 128bit tests given I'm sure by now type legalisation is already well tested for NEON.

878

As above I'm not sure of the value of these CHECK lines.

This revision is now accepted and ready to land.Feb 1 2022, 4:53 PM
sdesmalen accepted this revision.Feb 2 2022, 12:22 AM

The code looks fine to me. For the tests I would prefer it if you removed the new VBITS_EQ_128 lines for the >128bit vector tests as I don't believe they're testing anything new that isn't already being tested by the 64 and 128 bit vector tests. More than that, because you're hardwiring all the output I can see it just being a source of pain as the tests are more likely to need to be manually updated after unrelated code generation changes, which is something that has previously annoyed people. If you think the checks offer real value then as a minimum please consider at least removing them from the v32i8 variants as they're likely to be the worst offenders.

+1