This is an archive of the discontinued LLVM Phabricator instance.

[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
214

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

226

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
134–139

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

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
220–225

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.

369–373

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

376

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

381–385

Same comment as with getMaxSVEVectorSizeInBits.

386–388

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
357

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
386–388

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
226

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
357

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
357

Fair enough.

367–373

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
367–376

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/