Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/CodeGen/CodeGenFunction.cpp | ||
---|---|---|
489–490 | Is this really what we want? Won't this enable fixed length codegen all of the time? | |
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp | ||
119–131 ↗ | (On Diff #359785) | I'm not sure that RISCV have made a commitment to use the vscale_range attribute yet have they? In either case I think they should be involved in a change like this. |
clang/lib/CodeGen/CodeGenFunction.cpp | ||
---|---|---|
489–490 | Fixed length codegen is tied to the minimum vscale value, so by using 0 here means nothing is known about the minimum vscale and thus fixed length codegen will be restricted to 128bit as is the case when no attribute is specified. |
clang/lib/CodeGen/CodeGenFunction.cpp | ||
---|---|---|
489–490 | Ah right ok, I'd missed that detail. Ignore me then! |
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp | ||
---|---|---|
119–131 ↗ | (On Diff #359785) | Perhaps it's worth this patch not removing getMaxVScale just yet but rather just AArch64's implementation? There would only need to be a minor change to LoopVectorize.cpp along the lines of if (!MaxVScale && TheFunction->hasFnAttribute(Attribute::VScaleRange.... That way getMaxVScale can be removed if/when no one needs it, which I hope is not too far away. If there is agreement to remove it then I imagine code similar to what you've done for SVE in CodeGenFunction.cpp will be needed for RISCV otherwise the patch will cause a regression in functionality. |
clang/lib/CodeGen/CodeGenFunction.cpp | ||
---|---|---|
488 | Is there any way we can check that the target is ARM before checking an "sve" on a target independent interface? If any other target uses the name "sve" this will trigger. |
Removed changes to RiscV code
Added check that target isAArch64 before adding default value vscale_range attribute
clang/lib/CodeGen/CodeGenFunction.cpp | ||
---|---|---|
491 | Thinking about this more, maybe these values should be returned from a new interface on TargetInfo? Maybe using Optional so you can default to None for other targets? This way you don't have AArch64 specific numbers hardcoded in a file that doesn't belong to AArch64? But maybe the damage was already done with the "/ 128" in the ArmSveVectorBits code above. Maybe you can unify both into a new TargetInfo function? Does TargetInfo have access to LangOpts? |
Added getVScaleRange interface to TargetInfo and removed related AArch64 specific code from CodeGenFunction.cpp
You'll want to rebase your patch on top of latest main, since D105432 moved around the call to getMaxVScale() in AArch64TargetTransformInfo.cpp.
clang/lib/Basic/Targets/AArch64.cpp | ||
---|---|---|
433 | nit: unnecessary curly braces. | |
clang/lib/CodeGen/CodeGenFunction.cpp | ||
486–487 | nit: vscale_range | |
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp | ||
1589–1590 | This can drop the Optional now. I also think asserting the attribute must be set is a bit of a strong requirement? Maybe we can return an Invalid instead if the attribute is not set. |
Rebased onto main, updated newly added AArch64 getMaxVScale usages to use IR attribute instead
@craig.topper can you share RISCV plans around supporting vscale_range? In essence we'd like to deprecate the TTI method and have LLVM IR contain all relevant information when is comes to interpreting vscale.
Currently the usage is minimal and so checking both interfaces is not too bad but they'll come a point when there's no TTI available and then only the side supporting vscale_range can be considered. There's also the LTO side of things where relying on opt/llc flags to set register widths becomes fragile.
As it happens I was playing around with adding support for that today at least to get us started. I was going to put something up for review based on this patch, assuming this gets merged first.
However in the TTI method we're currently making use of some RISCVSubtarget properties to help refine the vscale range, so we'll need to think about how we'll deal with those.
clang/lib/Basic/Targets/AArch64.h | ||
---|---|---|
100 | This clang-tidy warning needs satisfied. |
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h | ||
---|---|---|
134 | Can this parameter be a Function*? given there's no real link between this function and LLVM Instructions. | |
142–143 | This can return 0 implying there is no know maximum. With the current code this means 0 will be returned instead of a sensible default. | |
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp | ||
5646 | I think you only want to set MaxVScale when VScaleRangeAttr.getVScaleRangeArgs().second is non-zero. Given this and the above similar comment perhaps there's need for extra tests that cover vscale_range(2,0) for example. |
Added checks for MaxVScale > 0
Changed getMaxNumElemenets() to take Function* instead of Instruction*
Fixed clang-tidy warning
I haven't included a test for vscale_range(2, 0) here, as one was added in the meantime in commit fe6ae81
Please remember to create a more representative commit message as the patch no longer removes getMaxVScale.
This clang-tidy warning needs satisfied.