[PATCH] D46749: [SelectionDAG]Reduce masked data movement chains and memory access widths

[Sam Parker via Phabricator via llvm-commits]

samparker added a comment.

Hi Diogo,

This is looking good, I have just a couple of inline queries. Also, please add a couple of negative tests that use a mask that isn't a multiple of 8 bits.

cheers,
sam



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8661
+    // Only accepts multiples of 8 bits, and power of 2 sizes
+    if (!MaxBit && (0 != (MaxBit | MinBit) % 8))
+      return SDValue();
----------------
OR instead of AND?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8665
+    unsigned ActiveBits = MaxBit - MinBit;
+    if (ActiveBits & (ActiveBits - 1))
       return SDValue();
----------------
Could you explain what you're checking here?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8668
 
-    unsigned ActiveBits = AndC->getAPIntValue().countTrailingOnes();
+    //    LLVM_DEBUG(dbgs() << "\tMask: 0x" << MaskAPInt.toString(16, false) <<
+    //    " : ";
----------------
remove or uncomment.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8707
+    ShAmt = MinBit;
+    ShLeftAmt = MinBit;
   }
----------------
Could you comment on why both of these are being set please?


================
Comment at: test/CodeGen/ARM/2018-ShiftedAndMask.ll:54
+
+define void @ldh(i32* %A) {
+entry:
----------------
Is it now possible to do a ldrh with an offset of 1 by using a mask of 0xff0?


https://reviews.llvm.org/D46749