This is an archive of the discontinued LLVM Phabricator instance.

[SveEmitter] Implement builtins for contiguous loads/stores
ClosedPublic

Authored by sdesmalen on Mar 16 2020, 10:34 AM.

Details

Summary

This adds builtins for all contiguous loads/stores, including
non-temporal, first-faulting and non-faulting.

Diff Detail

Event Timeline

sdesmalen created this revision.Mar 16 2020, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2020, 10:34 AM

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?

sdesmalen marked 2 inline comments as done.Mar 19 2020, 4:31 AM
sdesmalen added inline comments.
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.

SjoerdMeijer added inline comments.Mar 19 2020, 5:20 AM
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.

efriedma added inline comments.Mar 19 2020, 11:38 AM
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.)

sdesmalen updated this revision to Diff 251619.Mar 20 2020, 6:35 AM
sdesmalen marked 5 inline comments as done.
  • Rebased patch.
  • Avoid use of llvm::PointerType::getElementType().
  • Renamed Zxt -> ZExt, plus added a comment to the flag in the arm_sve.td file.
sdesmalen marked 4 inline comments as done.Mar 20 2020, 6:41 AM
sdesmalen added inline comments.
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.

SjoerdMeijer accepted this revision.Mar 20 2020, 7:49 AM

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)

This revision is now accepted and ready to land.Mar 20 2020, 7:49 AM
sdesmalen updated this revision to Diff 251664.Mar 20 2020, 9:11 AM
sdesmalen marked an inline comment as done.
  • 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.
sdesmalen updated this revision to Diff 254144.Apr 1 2020, 3:13 AM

Removed the _shortform.c files, in favour of describing the overloaded forms with a macro.

@SjoerdMeijer are you happy with the changes I've made to the tests?

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

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.

Andrzej added a subscriber: Andrzej.Apr 7 2020, 6:59 AM
Andrzej added inline comments.
clang/include/clang/Basic/arm_sve.td
186

Tests for ldff1sb seem to be missing.

This revision was automatically updated to reflect the committed changes.
sdesmalen marked an inline comment as done.
sdesmalen added inline comments.Apr 14 2020, 7:30 AM
clang/include/clang/Basic/arm_sve.td
186

Good spot, I've added this test before committing the patch.

clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ld1.c