Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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
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. |
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
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 |