[PATCH] D33715: [PPC] exploit rotate-left-then-mask-insert instructions for bitfield insert

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:1182
     int64_t BM = Inst.getOperand(3).getImm();
-    if (!isRunOfOnes(BM, MB, ME))
+    if (!isRunOfOnes<unsigned>(BM, MB, ME))
       break;
----------------
Do we not want to use `uint32_t` instead of `unsigned` to emphasize that this is a 32-bit value?


================
Comment at: lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h:75
+
+  if (Is64Bit?isShiftedMask_64(Val):isShiftedMask_32(Val)) {
     // look for the first non-zero bit
----------------
Nit: spaces between binary operators and operands.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:700
+  // Typically, tryBitPermutation can handle complicated case
+  // with overwrapping masks better.
+  if (SimpleCaseOnly && (TargetMask & InsertMask) != 0)
----------------
Overwrapping? It seems you're checking to ensure they don't overlap (i.e. no bits in common).


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:704
+
+  if ((TargetMask | InsertMask) == 0xFFFFFFFFFFFFFFFFuLL) {
+    uint64_t Op0Opc = Op0.getOpcode();
----------------
Hard to count all the F's, please use a simpler expression. Maybe `~0ULL` or even `~(TargetMask | InsertMask) == 0ULL)` if it's all of them (or something along those lines.

Also, a comment about why this condition is being checked.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2885
+  // since tryBitPermutation handles complicated bit manipulation better.
+  if (N->getOpcode() == ISD::OR)
+    if ((N->getValueType(0) == MVT::i32 && tryBitfieldInsert32(N, true)) ||
----------------
Between this and the early exits in the functions that find Rotate-and-insert opportunities, it seems that the simple case is `(or (or %a, %b) (or %c, %d))` where the two operands of the outer `or` have known-zero bits in complementary locations.
If that's the case, please add a comment for this and an explanation why this case is special. Furthermore (as Kit has already alluded to) it is likely that this simple case has simple handling and should get a corresponding simple function rather than passing a `bool` parameter to these functions to tell them where they're called from.


https://reviews.llvm.org/D33715