nemanjai requested changes to this revision. nemanjai added inline comments. This revision now requires changes to proceed.
================ Comment at: clang/include/clang/Sema/Sema.h:12559 bool SemaBuiltinOSLogFormat(CallExpr *TheCall); + bool CheckPPCisRunOfOnes(CallExpr *TheCall, unsigned ArgNum); + bool CheckPPC64isRunOfOnes(CallExpr *TheCall, unsigned ArgNum); ---------------- I don't think these names adequately describe what the check is. We are not checking if `PPC/PPC64` is a run of ones. In fact, the concept of a contiguous mask is in no way specific to PPC so we don't really need to differentiate this as a PPC-specific check. Also, the convention seems to be to prefix all names with `Sema`. Let's not part with that convention. Perhaps `SemaValueIsRunOfOnes()` or `SemaArgIsRunOfOnes()`. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:3299 + unsigned Val = Result.getExtValue(); + if (!Val) + return Diag(TheCall->getBeginLoc(), ---------------- Isn't a zero technically a contiguous run of ones (of length zero)? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:3429 SemaBuiltinConstantArgRange(TheCall, 0, 0, 1); + case PPC::BI__builtin_ppc_rlwnm: + return SemaBuiltinConstantArg(TheCall, 1, Result) || ---------------- Please describe in a comment why we need to ensure that the argument is a contiguous mask. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:3430 + case PPC::BI__builtin_ppc_rlwnm: + return SemaBuiltinConstantArg(TheCall, 1, Result) || + CheckPPCisRunOfOnes(TheCall, 2); ---------------- We seem to populate the `Result` here. If we already have the `Result` (i.e. the constant value) can't we just simply check if that is a run of ones? Then we can greatly simplify `CheckPPCisRunOfOnes()` and `CheckPPC64isRunOfOnes()` (and we may not actually need two functions since `Result` will be an `APSInt` that knows how wide it is. We could presumably just have: `bool SemaValueIsRunOfOnes(const APSInt &Val, unsigned Width);` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104744/new/ https://reviews.llvm.org/D104744 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits