Page MenuHomePhabricator

[IR] Add vscale_range IR function attribute
ClosedPublic

Authored by bsmith on Mar 5 2021, 3:26 AM.

Details

Summary

This attribute represents the minimum and maximum values vscale can
take. For now this attribute is not hooked up to anything during
codegen, this will be added in the future when such codegen is
considered stable.

Additionally hook up the -msve-vector-bits=<x> clang option to emit this
attribute.

Diff Detail

Event Timeline

bsmith created this revision.Mar 5 2021, 3:26 AM
bsmith requested review of this revision.Mar 5 2021, 3:26 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 5 2021, 3:26 AM

Thanks for this patch, it's great to have an IR attribute for this. The mechanics of this patch look mostly fine to me.
I added a few more subscribers to give this some wider visibility.

llvm/docs/LangRef.rst
1998

Do you need to say anything about a default if the attribute is not set, e.g. if vscale_range is not set, no assumptions are made about the range of vscale and the compiler falls back on the architectural maximum (if available).

llvm/lib/IR/Attributes.cpp
564–565

nit: unsigned MinValue, MaxValue;

paulwalker-arm added inline comments.Mar 5 2021, 4:46 AM
llvm/docs/LangRef.rst
1998

When the attribute is omitted there is no implicit knowledge and thus I'd stop at "... no assumptions are made about the range of vscale."

I would have though such a condition is implicit across most attributes (i.e. if omitted then no extra information is available) but I guess it cannot hurt to be explicit.

peterwaller-arm added inline comments.Mar 8 2021, 9:47 AM
llvm/lib/IR/Attributes.cpp
570

Nit: The only other precedent I can see for this is allocsize. Grepping the code I found this is always written in tests as allocsize(x, y), however, it prints as allocsize(x,y) (no space) when done with -emit-llvm, regardless of how it was formatted as input. I figure that is an oversight and this should have the space.

bsmith updated this revision to Diff 330205.Mar 12 2021, 4:10 AM
bsmith marked 3 inline comments as done.
  • State what lack of vscale_range attribute means in LangRef
  • Minor formatting change
llvm/lib/IR/Attributes.cpp
570

I'm reluctant to change allocsize and given that is the only other example of multiple parameters to an attribute like this, I'm inclined to stay consistent with it. I also think there is an argument to be made about not intruding extra spaces in a potentially already cluttered attributes section.

sdesmalen accepted this revision.Mar 12 2021, 4:23 AM

Looks fine to me, thanks.

This revision is now accepted and ready to land.Mar 12 2021, 4:23 AM
paulwalker-arm added inline comments.Mar 15 2021, 6:45 AM
clang/test/CodeGen/arm-sve-vector-bits-vscale-range.c
11–15

I'm happy with this but for information you could use the same trick as for the llvm/test/Analysis/CostModel/AArch64/sve-fixed-length.ll so that only a single CHECK, and CHECK-NOT line is required.

llvm/include/llvm/IR/Attributes.h
945

a

llvm/test/Verifier/vscale_range.ll
4–5

I think attributes.ll is a better place for this test, along with a CHECK for it's expected output.

Kind of related, what is expected for vscale_range(0,0) and does that need a test (positive or negative)?

bsmith updated this revision to Diff 330691.Mar 15 2021, 9:30 AM
bsmith marked 3 inline comments as done.
  • Prevent vscale_range(0,0) from crashing and instead don't add the attribute
  • Improve CHECK lines in arm-sve-vector-bits-vscale-range.c test
  • Test vscale_range(0,0) case and move some tests around
paulwalker-arm accepted this revision.Mar 15 2021, 11:46 AM
This revision was landed with ongoing or failed builds.Mon, Mar 22, 5:05 AM
This revision was automatically updated to reflect the committed changes.