[PATCH] D103702: [AArch64][SVE] Wire up vscale_range attribute to SVE min/max vector queries
Sander de Smalen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 9 01:38:39 PDT 2021
sdesmalen added a comment.
Just left a new nits, but otherwise looks fine to me.
================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:214
+ const TargetMachine &TM, bool LittleEndian,
+ unsigned MinSVEVectorSizeInBitsOverride,
+ unsigned MaxSVEVectorSizeInBitsOverride)
----------------
nit: is it worth making these Optional, if the subtarget doesn't have SVE?
================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:226
+ : SVEVectorBitsMaxOpt),
TargetTriple(TT), FrameLowering(),
InstrInfo(initializeSubtargetDependencies(FS, CPU)), TSInfo(),
----------------
nit: Does this need an assert that Min|MaxSVEVectorSizeInBits is zero when SVE is not enabled in the feature string?
================
Comment at: llvm/test/CodeGen/AArch64/sve-vscale-attr.ll:134-139
+attributes #0 = { "target-features"="+sve" }
+attributes #1 = { "target-features"="+sve" vscale_range(1,1) }
+attributes #2 = { "target-features"="+sve" vscale_range(2,2) }
+attributes #3 = { "target-features"="+sve" vscale_range(2,4) }
+attributes #4 = { "target-features"="+sve" vscale_range(4,4) }
+attributes #5 = { "target-features"="+sve" vscale_range(8,8) }
----------------
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103702/new/
https://reviews.llvm.org/D103702
More information about the llvm-commits
mailing list