[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 looks like two patches to me so please separate them. I would also expect to see more tests, this is quite a large change so try to cover some of the corner cases.

Also, one styling comment is that we use capital case for variables.

cheers,
sam



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4017
+// x2 = shl x1, 8 ; reuse the computation of x1
+SDValue DAGCombiner::combine_AND_ShiftAND(SDNode *N, SDValue &N0, SDValue &N1) {
+  ConstantSDNode *mask = dyn_cast<ConstantSDNode>(N1);
----------------
N is an AND, right? This would be easier to read if just N is passed, which you can then add an assert on the opcode, and then you can make N0 and N1 local variables.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4029
+
+  const ISD::NodeType N0Opcode = (ISD::NodeType)N0.getOpcode();
+  if (((N0Opcode < ISD::SHL) || (N0Opcode > ISD::ROTR)) &&
----------------
unsigned is fine, otherwise try to avoid using C-style casting. It would also be more readable if it was called something like 'ShiftOpcode'.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4030
+  const ISD::NodeType N0Opcode = (ISD::NodeType)N0.getOpcode();
+  if (((N0Opcode < ISD::SHL) || (N0Opcode > ISD::ROTR)) &&
+      ((N0Opcode < ISD::SHL_PARTS) || (N0Opcode > ISD::SRL_PARTS)))
----------------
Don't rely on the ordering of these values. How about using the switch below to default to returning instead of an unreachable?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4036
+  for (SDNode *otherUser : maskedValue->uses()) {
+    SDNode *shiftOperand = dyn_cast<SDNode>(N0);
+    if ((shiftOperand == nullptr) || (&(*otherUser) == shiftOperand) or
----------------
Pull the cast out of the loop, the nullptr check is also unnecessary.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4055
+      break;
+    case ISD::SRA:
+    case ISD::SRL:
----------------
Is SRA really legal here?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4065
+      break;
+    case ISD::SHL_PARTS:
+    case ISD::SRA_PARTS:
----------------
Best to just have a TODO comment and remove these opcodes from the switch.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4082
+      SDValue newShift =
+          DAG.getNode(N0Opcode, DL, VT, shiftTheAND, N0.getOperand(1));
+      AddToWorklist(maskedValue);
----------------
You've already created a local var for 'shiftAmount', it would be easier to follow if you continued to use it.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4083
+          DAG.getNode(N0Opcode, DL, VT, shiftTheAND, N0.getOperand(1));
+      AddToWorklist(maskedValue);
+      AddToWorklist(otherUser);
----------------
Don't think you need to add these as they're not new.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8649
     auto AndC = dyn_cast<ConstantSDNode>(N->getOperand(1));
-    if (!AndC || !AndC->getAPIntValue().isMask())
+    const APInt &maskAPInt = AndC->getAPIntValue();
+    // TODO: Not only [shifted] masks should be accepted.
----------------
AndC could be a nullptr here.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12991
 }
+/// Detects operations such as M[i] = M[i] | M[i] << K, or
+///                            M[i] = M[i] | M[i] >> K,
----------------
This looks like two bits of new functionality, so please separate them into two patches.


================
Comment at: test/CodeGen/ARM/stld-width-reduction1.ll:20
+
+attributes #0 = { norecurse nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="arm7tdmi" "target-features"="+armv4t,+strict-align,-thumb-mode" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
----------------
Please remove the attributes and metadata.


https://reviews.llvm.org/D46749