[PATCH] D148710: [DAGCombiner] Limit 'hoist add/sub binop w/ constant op' to the same divergency property

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3874
   // Hoist one-use addition by non-opaque constant:
-  //   (x + C) - y  ->  (x - y) + C
-  if (N0.getOpcode() == ISD::ADD && N0.hasOneUse() &&
-      isConstantOrConstantVector(N0.getOperand(1), /*NoOpaques=*/true)) {
-    SDValue Sub = DAG.getNode(ISD::SUB, DL, VT, N0.getOperand(0), N1);
-    return DAG.getNode(ISD::ADD, DL, VT, Sub, N0.getOperand(1));
-  }
-  // y - (x + C)  ->  (y - x) - C
-  if (N1.getOpcode() == ISD::ADD && N1.hasOneUse() &&
-      isConstantOrConstantVector(N1.getOperand(1), /*NoOpaques=*/true)) {
-    SDValue Sub = DAG.getNode(ISD::SUB, DL, VT, N0, N1.getOperand(0));
-    return DAG.getNode(ISD::SUB, DL, VT, Sub, N1.getOperand(1));
-  }
-  // (x - C) - y  ->  (x - y) - C
-  // This is necessary because SUB(X,C) -> ADD(X,-C) doesn't work for vectors.
-  if (N0.getOpcode() == ISD::SUB && N0.hasOneUse() &&
-      isConstantOrConstantVector(N0.getOperand(1), /*NoOpaques=*/true)) {
-    SDValue Sub = DAG.getNode(ISD::SUB, DL, VT, N0.getOperand(0), N1);
-    return DAG.getNode(ISD::SUB, DL, VT, Sub, N0.getOperand(1));
-  }
-  // (C - x) - y  ->  C - (x + y)
-  if (N0.getOpcode() == ISD::SUB && N0.hasOneUse() &&
-      isConstantOrConstantVector(N0.getOperand(0), /*NoOpaques=*/true)) {
-    SDValue Add = DAG.getNode(ISD::ADD, DL, VT, N0.getOperand(1), N1);
-    return DAG.getNode(ISD::SUB, DL, VT, N0.getOperand(0), Add);
+  if (N0->isDivergent() == N1->isDivergent()) {
+    //   (x + C) - y  ->  (x - y) + C
----------------
bcl5980 wrote:
> foad wrote:
> > We also want to do this if x is divergent and y is uniform.
> If x is divergent and y is uniform, we can transform to 
> (x + C) - y --> x + (C - y)
> y - (x + C) --> (y - C) - x
> (x - C) -y   --> x - (C + y) ;; this is speical case that AMDGPU doesnt' support op1 as sgpr, so this case we needn't optimize it. But this should be a TLI behaivor. Generally I think we still can't enable it.
> (C - x) - y --> (C -y) -x
> 
> So I believe in DAGCombiner we only need to enable it when the divergency property is the same.
> 
> 
The rules we implement in DAGCombiner should make sense without having to understand exactly what any particular target can do.

The original rule here was "pull constants out of nested adds/subs".

The new rule is "pull constants out of nested adds/subs, unless that increases the number of divergent nodes in the dag".


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148710/new/

https://reviews.llvm.org/D148710