This is an archive of the discontinued LLVM Phabricator instance.

[SVE][Inline-Asm] Add constraints for SVE predicate registers
ClosedPublic

Authored by kmclaughlin on Aug 21 2019, 4:12 AM.

Details

Summary

Adds the following inline asm constraints for SVE:

  • Upl: One of the low eight SVE predicate registers, P0 to P7 inclusive
  • Upa: SVE predicate register with full range, P0 to P15

Diff Detail

Repository
rL LLVM

Event Timeline

kmclaughlin created this revision.Aug 21 2019, 4:12 AM
Herald added a project: Restricted Project. · View Herald Transcript
sdesmalen added inline comments.Aug 21 2019, 5:53 AM
lib/IR/InlineAsm.cpp
188 ↗(On Diff #216351)

"Expected a digit" seems more appropriate, since this code is only testing a single character.

192 ↗(On Diff #216351)

StringRef can be used here instead of std::string.

lib/Target/AArch64/AArch64ISelLowering.cpp
5803 ↗(On Diff #216351)

This is missing a check that Constraint.size() == 3. Or perhaps it is even better to split this code out into a separate function, because this patch writes out the condition three times. Maybe something like PredicateConstraint isPredicateConstraint(StringRef S), where PredicateConstraint is an enum.

  • Added isPredicateConstraint function to AArch64ISelLowering.cpp, which returns Upl, Upa or Invalid. This is used to replace some repeated checks of the predicate type
  • Minor changes to InlineAsm.cpp
rovka added a comment.Sep 2 2019, 2:24 AM

Just some drive-by suggestions :)

lib/Target/AArch64/AArch64ISelLowering.cpp
5747 ↗(On Diff #218071)

Nit: I think get- or parsePredicateConstraint reads better, since this doesn't return a simple yes/no answer.

test/CodeGen/AArch64/aarch64-sve-asm.ll
50 ↗(On Diff #218071)

Nit: I would be a bit pedantic here and also check that they are used for the inline asm.

greened added inline comments.Sep 3 2019, 7:31 AM
docs/LangRef.rst
3818 ↗(On Diff #218071)

What do these names mean? "<something> predicate lower|all?" I see they are the names used in gcc, so I guess it makes sense to use them here. Are these names used in the SVE architecture manual somewhere? I cannot find them.

lib/Target/AArch64/AArch64ISelLowering.cpp
5747 ↗(On Diff #218071)

+1.

  • Renamed the isPredicateConstraint function to parsePredicateConstraint
  • Added more thorough checks to the tests in aarch64-sve-asm.ll
kmclaughlin marked 3 inline comments as done.Sep 10 2019, 5:38 AM
kmclaughlin added inline comments.
docs/LangRef.rst
3818 ↗(On Diff #218071)

The length of the constraint depends on the first letter, and for AArch64 'U' was chosen to indicate a 3-letter constraint. These are not in the SVE architecture manual as they are only for compatibility with GCC inline asm (see https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html#Machine-Constraints)

lib/Target/AArch64/AArch64ISelLowering.cpp
5747 ↗(On Diff #218071)

Thanks for the suggestion @rovka - I have changed this to parsePredicateConstraint

rovka accepted this revision.Sep 16 2019, 2:04 AM

I think all the outstanding comments have been addressed. LGTM.

This revision is now accepted and ready to land.Sep 16 2019, 2:04 AM
This revision was automatically updated to reflect the committed changes.