Page MenuHomePhabricator

[AArch64][SVE] Wire up vscale_range attribute to SVE min/max vector queries
ClosedPublic

Authored by bsmith on Jun 4 2021, 9:16 AM.

Diff Detail

Event Timeline

bsmith created this revision.Jun 4 2021, 9:16 AM
bsmith requested review of this revision.Jun 4 2021, 9:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2021, 9:16 AM
Matt added a subscriber: Matt.Jun 4 2021, 9:17 AM
bsmith updated this revision to Diff 349892.Jun 4 2021, 9:18 AM
  • Fix minor mistake in test
joechrisellis accepted this revision.Jun 9 2021, 1:18 AM

Looks good to me. 😄

Do you think it might also be worth adding a clang test so that we can verify that the -msve-vector-bits=N flag is working end-to-end?

This revision is now accepted and ready to land.Jun 9 2021, 1:18 AM

Just left a new nits, but otherwise looks fine to me.

llvm/lib/Target/AArch64/AArch64Subtarget.cpp
202

nit: is it worth making these Optional, if the subtarget doesn't have SVE?

207–208

nit: Does this need an assert that Min|MaxSVEVectorSizeInBits is zero when SVE is not enabled in the feature string?

llvm/test/CodeGen/AArch64/sve-vscale-attr.ll
135–140

nit: I'm not sure if the attributes need to be at the end of the file necessarily, but if not, can you move each of them below the function that uses it?

Matt added inline comments.Jun 9 2021, 5:27 PM
llvm/unittests/Target/AArch64/InstSizes.cpp
33 ↗(On Diff #349892)

Nit: Adding parameter comments similar to the existing /* isLittle */ false may be a bit clearer than 0, 0.

paulwalker-arm added inline comments.Jun 10 2021, 5:06 AM
llvm/lib/Target/AArch64/AArch64Subtarget.cpp
208–213

What about moving the aarch64-sve-vector-bits command line opts into AArch64TargetMachine.cpp and resolving the conflict there? That way all the logic is in one place.

349–350

Based on my previous comment I think these two asserts can be move into AArch64TargetMachine.cpp also, and/or perhaps AArch64Subtarget::AArch64Subtarget.

349–350

Same comment as with getMaxSVEVectorSizeInBits.

349–350

Again assuming my "move opts to AArch64TargetMachine.cpp" suggestion is sound I think this "normalise user input" code can sit in AArch64TargetMachine.cpp also.

349–350

Based on my other comments I'm wondering if we just got to the point where both these functions need to be moved into AArch64TargetMachine.cpp with AArch64Subtarget having simple accessors.

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
369

I don't know if this is possible but I feel we need a HasSVE like check here?

bsmith added inline comments.Jun 14 2021, 4:46 AM
llvm/lib/Target/AArch64/AArch64Subtarget.cpp
349–350

I don't think we can go quite this far as AArch64TargetMachine is not function specific like the attributes are. I think we can move all of the logic of resolving between the options and the attribute, but the storage/accessors for the resulting value needs to live in the subtarget.

bsmith updated this revision to Diff 351910.Jun 14 2021, 10:10 AM
bsmith marked 7 inline comments as done.
  • Move attribute/command line logic into AArch64TargetMachine.
  • Fix issue with subtarget key appending integers.
  • Add end-to-end test.
  • Make new subtarget options optional.
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2021, 10:10 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
bsmith added inline comments.Jun 14 2021, 10:10 AM
llvm/lib/Target/AArch64/AArch64Subtarget.cpp
207–208

In principal you could, however I'm not sure it adds any value, as the accessors already assert that SVE is enabled. (And in principal this is a generic attribute, not an SVE one).

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
369

I'm not sure this is really doable here without picking apart the feature string, I think it makes more sense to just set the values and assert when using the accessors without SVE enabled.

llvm/lib/Target/AArch64/AArch64Subtarget.h
298–299

Out of interest are these defaults ever relied upon?

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
369

Fair enough.

379–385

These asserts are fine but you'll see from the original implementations of getM..SVEVectorSizeInBits that I do not rely on the user passing the correct values. Instead I also always process the sizes to ensure the values of MinSVEVectorSize and MaxSVEVectorSizeare sane. Can you do likewise here?

bsmith updated this revision to Diff 352101.Jun 15 2021, 5:11 AM
bsmith marked an inline comment as done.
  • Ensure user input is sanitized for when asserts are not enabled
  • Fix clang format issues
llvm/lib/Target/AArch64/AArch64Subtarget.h
298–299

Only in the case of manual construction of a subtarget, for example a unit test etc. For normal cases this will always be passed in based on the attribute or command line options.

paulwalker-arm accepted this revision.Jun 18 2021, 5:35 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64Subtarget.cpp
349–350

Up to you but now these are simple accessors, is it worth having the implementations inlined into the header?

This commit is failing the test on non-arm targets (e.g. https://lab.llvm.org/buildbot/#/builders/139/builds/5932). I think the test needs a 'requires' line for aarch64.

Yup, just committed a fix in ed31ff9c7a9e538ead1fa4feecf09987998621b4, sorry for the noise.

Matt added inline comments.Jun 21 2021, 8:38 AM
llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
161

Just noticed a small typo: s/AAcrh64/AArch64/