Page MenuHomePhabricator

[SVE] Remove usage of getMaxVScale for AArch64, in favour of IR Attribute
ClosedPublic

Authored by DylanFleming-arm on Jul 19 2021, 7:25 AM.

Diff Detail

Event Timeline

DylanFleming-arm requested review of this revision.Jul 19 2021, 7:26 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 19 2021, 7:26 AM
DylanFleming-arm retitled this revision from [SVE] Remove the interface for in favour of the IR attributes to [SVE] Remove the interface for getMaxVScale in favour of the IR attributes.
bsmith added inline comments.
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.

paulwalker-arm added inline comments.Jul 19 2021, 7:46 AM
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.

bsmith added inline comments.Jul 19 2021, 7:48 AM
clang/lib/CodeGen/CodeGenFunction.cpp
489–490

Ah right ok, I'd missed that detail. Ignore me then!

Matt added a subscriber: Matt.Jul 19 2021, 1:20 PM
paulwalker-arm added inline comments.Jul 20 2021, 9:20 AM
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.

craig.topper added inline comments.Jul 22 2021, 8:15 AM
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

craig.topper added inline comments.Jul 22 2021, 11:47 AM
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.

@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.

frasercrmck added inline comments.Aug 2 2021, 9:10 AM
clang/lib/Basic/Targets/AArch64.h
100

This clang-tidy warning needs satisfied.

paulwalker-arm added inline comments.Aug 3 2021, 10:05 AM
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
5642

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

paulwalker-arm accepted this revision.Aug 13 2021, 3:05 AM
This revision is now accepted and ready to land.Aug 13 2021, 3:05 AM

Please remember to create a more representative commit message as the patch no longer removes getMaxVScale.

This revision was landed with ongoing or failed builds.Aug 17 2021, 6:43 AM
This revision was automatically updated to reflect the committed changes.
DylanFleming-arm retitled this revision from [SVE] Remove the interface for getMaxVScale in favour of the IR attributes to [SVE] Remove usage of getMaxVScale for AArch64, in favour of IR Attribute.Aug 17 2021, 6:45 AM