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

[Sam Parker via Phabricator via llvm-commits]

samparker added a comment.

Hi Diogo,

Some nitpick comments. Please add negative tests for shifts with multiple uses and where the shift and mask aren't by constants.

cheers,
sam



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4115
+  const SDValue &SHIFT = AND->getOperand(0);
+  if ((SHIFT.getNumOperands() != 2) || (!SHIFT.hasOneUse()))
+    return SDValue();
----------------
Why not check that you have a supported shift here and potentially exit early?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4154
+
+//    LLVM_DEBUG(
+//        dbgs() << "\tValue being masked and shif-masked: "; MASKED.dump();
----------------
Remove or uncomment.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4180
+        N0Opcode = ISD::SRL;
+      /* fall-thru */
+    case ISD::SRL:
----------------
use LLVM_FALLTHROUGH


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4189
+    case ISD::ROTL:
+//      CanReduce = (EffectiveOtherMask.rotl(ShiftValue) == EffectiveMask);
+//      break;
----------------
remove


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4203
+          DAG.getNode(N0Opcode, DL, VT, ShiftTheAND, SHIFT.getOperand(1));
+      AddToWorklist(OtherUser);
+      return NewShift;
----------------
I think you should be adding NewShift instead.


================
Comment at: test/CodeGen/ARM/2018_05_29_FoldRedundantMask.ll:119
+; CHECK: ldrh [[R:r[0-9]+]], [r{{[0-9]+}}]
+; ldr     r1, .LCPI5_0
+;	CHECK-NEXT: ldr	[[R1:r[0-9]+]], [[C:\.[A-z0-9_]+]]
----------------
Would you mind updating the target triple to something more modern, like v7 and v7m, to check that the code generation is better here. This extra load concerns me a bit.


Repository:
  rL LLVM

https://reviews.llvm.org/D48278