[PATCH] D127910: [Clang][AArch64][SME] Add vector load/store (ld1/st1) intrinsics

[Bryan Chan via Phabricator via cfe-commits]

bryanpkc marked an inline comment as done.
bryanpkc added inline comments.


================
Comment at: clang/lib/Headers/CMakeLists.txt:332
+  # Generate arm_sme.h
+  clang_generate_header(-gen-arm-sme-header arm_sme.td arm_sme.h)
   # Generate arm_bf16.h
----------------
sdesmalen wrote:
> bryanpkc wrote:
> > bryanpkc wrote:
> > > sdesmalen wrote:
> > > > The ACLE specification is still in a draft (ALP) state, which means there may still be subject to significant changes. To avoid users from using this implementation with the expectation that their code is compliant going forward, it would be good to rename the header file to something that makes it very clear this feature is not yet ready to use. I'm thinking of something like `arm_sme_draft_spec_subject_to_change.h`. When the specification goes out of draft, we can rename it to `arm_sme.h`. Could you rename the file for now?
> > > Would something shorter like `arm_sme_draft.h` or `arm_sme_experimental.h` suffice?
> > Renamed to `arm_sme_experimental.h`.
> While `arm_sme_experimental.h` is indeed shorter, it should be unequivocally clear to the user that they shouldn't rely on the function prototypes defined in this header file yet because the specification itself is not finalised. I think that adding the `draft_spec_subject_to_change` to the name makes that more clear than `experimental`, as the latter might suggest that the support is not yet entirely stable or complete. There probably isn't that much to gain from making the user experience better by using a shorter name, if the whole point is to prevent people from using it for any production code. So from that perspective, I still have a slight preference for `arm_sme_draft_spec_subject_to_change.h`.
Renamed as suggested.


================
Comment at: clang/test/Sema/aarch64-sme-intrinsics/acle_sme_imm.cpp:3
+
+// RUN: %clang_cc1 -D__ARM_FEATURE_SME -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +sme -fsyntax-only -verify -verify-ignore-unexpected=error %s
+// RUN: %clang_cc1 -D__ARM_FEATURE_SME -DSVE_OVERLOADED_FORMS -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +sme -fsyntax-only -verify -verify-ignore-unexpected=error %s
----------------
david-arm wrote:
> I think you can remove the `-target-feature +sve` flags from the RUN lines because `+sme` should imply that.
If I understand `llvm/lib/Target/AArch64/AArch64.td` and `clang/lib/Basic/Targets/AArch64.cpp` correctly, `-target-feature +sme` does not imply `-target-feature +sve`. I can remove the `-D__ARM_FEATURE_SME` though.

On the other hand, `-march=armv8+sme` will imply `-target-feature +sve`, which I have implemented in D142702.


================
Comment at: clang/utils/TableGen/SveEmitter.cpp:1477
+
+  OS << "#if !defined(__ARM_FEATURE_SME)\n";
+  OS << "#error \"SME support not enabled\"\n";
----------------
dmgreen wrote:
> bryanpkc wrote:
> > dmgreen wrote:
> > > We have been changing how the existing SVE and NEON instrinsics work a little. There are some details in https://reviews.llvm.org/D131064. The short version is it is best to not rely upon preprocessor macros, and instead define the intrinsics so that they can be used if the right target features are available. This allows us to do things like this below, even without a -march that supports sme, and have them callable at runtime under the right situations. We should be doing the same for SME.
> > > ```
> > > __attribute__((target("+sme")))
> > > void sme_func() {
> > >   somesmeintrinsics();
> > > }
> > > ```
> > I have refactored the SME intrinsics in a similar fashion. Could you confirm if I did it correctly?
> It looks OK - as far as I can see. It might be worth adding a test for it? But otherwise it looks good. Thanks.
Added a test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127910