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