This adds builtins for all contiguous loads/stores, including
non-temporal, first-faulting and non-faulting.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Some nits inlined
clang/include/clang/Basic/AArch64SVETypeFlags.h | ||
---|---|---|
72 ↗ | (On Diff #250595) | nit: this one is non obvious (at least to me), so perhaps worth a comment what this is. I can guess that Zext means zero extending, but you know, the context... |
clang/include/clang/Basic/arm_sve.td | ||
168 | nit: don't think we have a coding style for tablegen, but it is exceeding 80 characters, even making this on phabricator a bit difficult to read, perhaps you can reshuffle this a bit. | |
clang/lib/CodeGen/CGBuiltin.cpp | ||
7530 | nit: and now looking at this, this can be a zero or sign extend, so Zxt is slightly misleading? |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
7530 | The default is sign-extend, so we added a flag for the case where a zero-extend is needed/expected. Are you suggesting to rename the flag or to add an extra flag for IsSxt? | |
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1.c | ||
315 | I can probably prune these tests a bit, when we generated these initially we also tested for code-gen, (hence the 129, which doesn't fit the immediate). This is properly tested in .ll tests in LLVM now, so I'll remove these. |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
7530 | I don't know to be honest, it was a nit anyway, but whatever you think is best. But just reading the name IsZxtReturn, I was only expecting a zero-extend. |
clang/include/clang/Basic/AArch64SVETypeFlags.h | ||
---|---|---|
72 ↗ | (On Diff #250595) | "Zxt" is not a standard abbreviation in LLVM: please use "Zext", or "ZeroExtend". |
clang/lib/CodeGen/CGBuiltin.cpp | ||
7534 | ReturnTy is unused? | |
7538 | Please avoid using getPointerElementType where possible... it will interfere with opaque pointer types work. (You should be able to get the element type from the AST.) |
- Rebased patch.
- Avoid use of llvm::PointerType::getElementType().
- Renamed Zxt -> ZExt, plus added a comment to the flag in the arm_sve.td file.
clang/include/clang/Basic/arm_sve.td | ||
---|---|---|
168 | I gave it a try to avoid these long lines, but found that this made the .td file a lot less readable. If you don't have a strong opinion on this, I'd prefer to keep it as-is. |
Looks good to me. Please wait a day in case Eli has more comments.
clang/include/clang/Basic/arm_sve.td | ||
---|---|---|
168 | Ok, thanks for trying. | |
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ld1sh.c | ||
6 | Nit: I really don't mind this, but this comment in its current form it doesn't add much (might as well remove it, or add a description, same for the other files) |
- Created tests for the shortform (overloaded) intrinsics.
- Removed redundant tests that were originally intended to test the resulting asm (and thus tested whether the right addressing mode was used given different immediates). Because that we only need to test the mapping of builtin -> LLVM intrinsic, these tests could be removed.
Removed the _shortform.c files, in favour of describing the overloaded forms with a macro.
Thanks for the ping, hadn't noticed the updates.
I may have also missed why you need the SVE_ACLE_FUNC macro. Can you quickly comment why you need that? Just asking because in my opinion it doesn't really make it more pleasant to read (so it's there for another reason).
On one of the other tickets (may have missed a notification there too) I asked why you need option -fallow-half-arguments-and-returns. Do you really need that, and why?
And last question is if you really need to pass these defines -D__ARM_FEATURE_SVE?
Thanks, these are all good questions!
In the previous revision of the patch there were separate test files for the short-forms (overloaded forms) of the tests. For example:
svint8_t svadd[_s8]_z(svbool_t pg, svint8_t op1, svint8_t op2)
has the fully specific intrinsic svadd_s8_z, but also the overloaded form svadd_z. With the SVE_ACLE_FUNC(..) macro we can test both variants with a different RUN line, one where the [_s] suffix is ignored, the other where all suffixes are concatenated, depending on whether SVE_OVERLOADED_FORMS is defined.
On one of the other tickets (may have missed a notification there too) I asked why you need option -fallow-half-arguments-and-returns. Do you really need that, and why?
I answered that on the other patch now. Sorry about that, forgot to click 'submit' earlier.
And last question is if you really need to pass these defines -D__ARM_FEATURE_SVE?
__ARM_FEATURE_SVE is automatically enabled by the compiler when the compiler implements the ACLE spec and +sve is specified. Given that the compiler doesn't yet implement the spec, these tests add the feature macro explicitly. Once all builtins are upstreamed, we'll update these tests by removing the explicit -D__ARM_FEATURE_SVE flag.
clang/include/clang/Basic/arm_sve.td | ||
---|---|---|
186 | Tests for ldff1sb seem to be missing. |
clang/include/clang/Basic/arm_sve.td | ||
---|---|---|
186 | Good spot, I've added this test before committing the patch. |
nit: don't think we have a coding style for tablegen, but it is exceeding 80 characters, even making this on phabricator a bit difficult to read, perhaps you can reshuffle this a bit.