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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
