[PATCH] D104744: [PowerPC] Add PowerPC rotate related builtins and emit target independent code for XL compatibility

[Nemanja Ivanovic via Phabricator via cfe-commits]

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