[PATCH] D154805: [DAGCombiner] Fold IEEE `fmul`/`fdiv` by Pow2 to `add`/`sub` of exp

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:3975
+  // This is only queried after we have verified the transform will be bitwise
+  // equals.
+  virtual bool optimizeFMulOrFDivAsShiftAddBitcast(SDNode *N, SDValue FPConst,
----------------
Describe/tag the purpose of each argument or use the same names in the patterns to make it more obvious.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:3981
+    (void)FPConst;
+    (void)IntPow2;
+  }
----------------
(style) Remove the (void) lines


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16152
+                        unsigned Depth, bool AssumeNonZero) {
+  if (!VT.isInteger())
+    return SDValue();
----------------
Can this happen? Make it an assert if you must.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16154
+    return SDValue();
+  static const unsigned MaxDepth = 6;
+  auto PeekThroughCastsAndTrunc = [](SDValue V) {
----------------
Remove MaxDepth and use SelectionDAG::MaxRecursionDepth


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16158
+      switch (V.getOpcode()) {
+      case ISD::BITCAST:
+      case ISD::TRUNCATE:
----------------
If we allow bitcast can't we end with cases where we've flipped between vectors/scalars or changed vectorelementcount?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16178
+    return SDValue();
+  }
+
----------------
You can use ISD::matchUnaryPredicate to support non-uniform vectors here - see isDivisorPowerOfTwo for a similar case.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16180
+
+  if (Depth++ >= MaxDepth)
+    return SDValue();
----------------
Standard practice in DAG is to increment Depth at a recursive functional call, not inside it.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16201
+      if (SDValue LogX =
+              takeLog2(DAG, DL, VT, Op.getOperand(0), Depth, AssumeNonZero))
+        return DAG.getNode(ISD::ADD, DL, VT, LogX,
----------------
Use Depth + 1 here and adjust the depth tolerance above


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16209
+    if (SDValue LogX =
+            takeLog2(DAG, DL, VT, Op.getOperand(1), Depth, AssumeNonZero))
+      if (SDValue LogY =
----------------
Use Depth + 1 here and adjust the depth tolerance above


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16211
+      if (SDValue LogY =
+              takeLog2(DAG, DL, VT, Op.getOperand(2), Depth, AssumeNonZero))
+        return DAG.getSelect(DL, VT, Op.getOperand(0), LogX, LogY);
----------------
Use Depth + 1 here and adjust the depth tolerance above


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16220
+    // log2(umax(X, Y)) != umax(log2(X), log2(Y)) (because overflow).
+    if (SDValue LogX = takeLog2(DAG, DL, VT, Op.getOperand(0), Depth,
+                                /*AssumeNonZero*/ false))
----------------
Use Depth + 1 here and adjust the depth tolerance above


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16222
+                                /*AssumeNonZero*/ false))
+      if (SDValue LogY = takeLog2(DAG, DL, VT, Op.getOperand(1), Depth,
+                                  /*AssumeNonZero*/ false))
----------------
Use Depth + 1 here and adjust the depth tolerance above


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16256
+    ConstOp = peekThroughBitcasts(N->getOperand(ConstOpIdx));
+    Pow2Op = peekThroughBitcasts(N->getOperand(1 - ConstOpIdx));
+    if (Pow2Op.getOpcode() != ISD::UINT_TO_FP &&
----------------
Don't you need to ensure that the scalar size in bits of stays the same through the bitcast? AFAICT you should only accept bitcasts that casts from fp to/from int (both scalar/vector and same scalar width)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154805