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

Kit Barton via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 5 11:12:42 PDT 2017


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

If I understand this correctly, you're trying to use the tryBitfieldInsert before the tryBitPermutation function because you can get cleaner code out of some cases of tryBitfieldInsert. However, you only want to use the tryBitfieldInsert on some cases, not on all. Is this correct?
If so, I think it makes sense to refactor the tryBitfieldInsert to focus on the specific cases you want to try and identify and then call the existing tryBitPermutation and the remaining tryBitfieldInsert afterwards. There may be some code duplication as a result (hopefully that can be minimal) but the logic will be easier to understand.



================
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 don't understand why this has been transformed into a templated function. Is this necessary for this patch, or just some kind of cleanup?


================
Comment at: lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h:75
+
+  if (Is64Bit?isShiftedMask_64(Val):isShiftedMask_32(Val)) {
     // look for the first non-zero bit
----------------
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. 


================
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);
----------------
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. 


https://reviews.llvm.org/D33715





More information about the llvm-commits mailing list