[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

Wed Apr 1 04:24:52 PDT 2020

sdesmalen added a comment.

In D76238#1954585 <https://reviews.llvm.org/D76238#1954585>, @SjoerdMeijer wrote:

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

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76238/new/

https://reviews.llvm.org/D76238