[PATCH] D127762: [Clang][AArch64] Add/implement ACLE keywords for SME.

Wed Aug 2 09:17:55 PDT 2023

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, though please give other active reviewers a bit of time to chime in before landing. Note, @erichkeane is out on sabbatical at the moment, so there's no need to wait for his LG (he can leave post-commit comments on the review when he gets back though, so keep your eyes peeled for that just in case).

================
Comment at: clang/include/clang/AST/Type.h:3987
     /// [implimits] 8 bits would be enough here.
-    uint16_t NumExceptionType = 0;
+    uint16_t NumExceptionType;
+
----------------
sdesmalen wrote:
> aaron.ballman wrote:
> > I think this should also be an `unsigned` bit-field so that it packs together with the new field added below. Otherwise we're liable to end up with padding between this field and the next bit-field (which will take a full allocation unit anyway). How about `unsigned NumExceptionType : 10;`? We're reducing the count by 6 bits, but.... I struggle to imagine code being impacted and we're still allowing more than the implementation limits require.
> That's what I did initially in the patch where I limited `NumExceptionType` to 16bits (D152140), but there was the preference to have this be a uint16_t instead. I've changed it back again to a bitfield though.
I'm sorry for the back and forth! I think Erich's suggestion was wrong in the other thread, mostly because of MSVC: https://godbolt.org/z/W9fn7Ghxo

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127762