[PATCH] D140222: [AArch64] Check 128-bit Sysreg Builtins

[Tomas Matheson via Phabricator via cfe-commits]

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

Couple of nits, since you will be updating this anyway after dropping D140221 <https://reviews.llvm.org/D140221>, otherwise LGTM.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:8297
              << Arg->getSourceRange();
   } else if (IsAArch64Builtin && Fields.size() == 1) {
+    // If this is a write ...
----------------
It might be more readable to outline this whole branch and remove the redundant "else".


================
Comment at: clang/lib/Sema/SemaChecking.cpp:8298
   } else if (IsAArch64Builtin && Fields.size() == 1) {
-    // If the register name is one of those that appear in the condition below
-    // and the special register builtin being used is one of the write builtins,
-    // then we require that the argument provided for writing to the register
-    // is an integer constant expression. This is because it will be lowered to
-    // an MSR (immediate) instruction, so we need to know the immediate at
-    // compile time.
+    // If this is a write ...
     if (TheCall->getNumArgs() != 2)
----------------
This comment style is a bit confusing; the actual code says if(NOT a write) return false; imo the code is readable enough without them.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:8334-8340
+    if (MaxLimit)
+      return SemaBuiltinConstantArgRange(TheCall, 1, 0, MaxLimit.value());
+
+    // Otherwise, no checking is needed as we we lower to some kind of MSR
+    // (register) rather than an MSR (immediate).
+    return false;
   }
----------------
nit: stick to early return pattern



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140222