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

[Hiroshi Inoue via Phabricator via llvm-commits]

inouehrs marked 7 inline comments as done.
inouehrs 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;
----------------
nemanjai wrote:
> Do we not want to use `uint32_t` instead of `unsigned` to emphasize that this is a 32-bit value?
I changed the type of `BM` instead of specifying type in `isRunOfOnes` since BM is used only in this function. I feel this is cleaner than explicitly saying uint32_t for isRunOfOnes.


================
Comment at: lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h:63
+template <typename T>
+static inline bool isRunOfOnes(T Val, unsigned &MB, unsigned &ME) {
+  static_assert(std::numeric_limits<T>::is_integer &&
----------------
kbarton wrote:
> I don't understand why this has been transformed into a templated function. Is this necessary for this patch, or just some kind of cleanup?
I need 64-bit version of `isRunOfOnes`. Since 32-bit and 64-bit versions are almost same, I use template to avoid writing almost same function twice.


================
Comment at: lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h:75
+
+  if (Is64Bit?isShiftedMask_64(Val):isShiftedMask_32(Val)) {
     // look for the first non-zero bit
----------------
kbarton wrote:
> nemanjai wrote:
> > Nit: spaces between binary operators and operands.
> I don't think I've ever seen a ternary used in an if statement like this.
> If we don't have precedent for this, could you please put spaces around the ? and :
> I find this difficult to read as is. 
I hope this version is easier to read.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:578
 /// mask insert (rlwimi) instruction.
-bool PPCDAGToDAGISel::tryBitfieldInsert(SDNode *N) {
+bool PPCDAGToDAGISel::tryBitfieldInsert32(SDNode *N, bool SimpleCaseOnly) {
   SDValue Op0 = N->getOperand(0);
----------------
kbarton wrote:
> Is it possible to refactor this to separate the SimpleCase from the non-simple case without requiring too much code duplication?
> This will get rid of the boolean parameter, which makes it harder to follow. 
I added a new `tryBitfieldInsert` for checking simple bitfield insert cases.
The original `tryBitfieldInsert` actually covers wider range than a simple bitfield insert; so I renamed it to `tryRotateThenMaskInsert`


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


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:704
+
+  if ((TargetMask | InsertMask) == 0xFFFFFFFFFFFFFFFFuLL) {
+    uint64_t Op0Opc = Op0.getOpcode();
----------------
nemanjai wrote:
> 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.
Done.


================
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)) ||
----------------
nemanjai wrote:
> 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.
As Kit and you suggested, I refactored the code to avoid a boolean parameter.


https://reviews.llvm.org/D33715