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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

The semantics of the patch seem perfectly fine. Please refactor the functions with duplicated code.



================
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 &&
----------------
I find it rather surprising that an adequate function doesn't exist in either `APInt` or somewhere in `MathExtras.h`. I wonder if adding it there might be a better place. Perhaps check with frequent contributors to those files?


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:713
+/// then mask insert (rldimi) instruction.
+bool PPCDAGToDAGISel::tryRotateThenMaskInsert64(SDNode *N) {
+  SDValue Op0 = N->getOperand(0);
----------------
It really seems like this is a bunch of unnecessary code duplication. The only difference in the first 70 lines of this function vs. `tryRotateThenMaskInsert32()` is replacing `32` with `64`. Since you have the bit width available from the parameter, I think you should unify these two functions into one (and if it makes more sense, split out the last few lines into a pair of small functions/lambda).


https://reviews.llvm.org/D33715