Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?
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? |
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. |
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? |
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. |
- Move attribute/command line logic into AArch64TargetMachine.
- Fix issue with subtarget key appending integers.
- Add end-to-end test.
- Make new subtarget options optional.
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? |
- 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. |
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.
llvm/lib/Target/AArch64/AArch64TargetMachine.cpp | ||
---|---|---|
161 | Just noticed a small typo: s/AAcrh64/AArch64/ |
Out of interest are these defaults ever relied upon?