[PATCH] D38313: [InstCombine] Introducing Aggressive Instruction Combine pass

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:76
+
+bool TruncInstCombine::buildTruncExpressionDag(Value *V, unsigned Depth) {
+  if (Depth > MaxDepth)
----------------
You should use a worklist here, not recursion. Then they'll be no need for the depth limit (or it can be very large).


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:113
+  default:
+    // TODO: Can handle more cases here.
+    return false;
----------------
You can mention specific things here. Comments like "TODO: more work" are not helpful. You have a list in your patch description:

> select, shufflevector, extractelement, insertelement
> udiv, urem
> shl, lshr, ashr
> phi node (and loop handling)


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:157
+  InstInfoMap.clear();
+  if (!buildTruncExpressionDag(Src, 0))
+    return nullptr;
----------------
If we have a DAG of instructions with multiple trunc outputs, we'll end up walking the DAG once per trunc output?


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:181
+    bool FromLegal = MinBitWidth == 1 || DL.isLegalInteger(OrigBitWidth);
+    bool ToLegal = MinBitWidth == 1 || DL.isLegalInteger(MinBitWidth);
+    if (!DstTy->isVectorTy() && FromLegal && !ToLegal)
----------------
If your goal is only to produce legal integer widths (which might miss some cases for vectorization), then I think that you should just fold this information into getMinBitWidth so that getMinBitWidth returns the minimum *legal* bit width.


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:281
+    default:
+      llvm_unreachable("Unreachable!");
+    }
----------------
Please write actual messages in llvm_unreachable. Perhaps something like:

  llvm_unreachable("Unhandled instruction");



https://reviews.llvm.org/D38313