[PATCH] D56214: AggressiveInstCombine: Fold full mul i64 x i64 -> i128

[Arthur O'Dwyer via Phabricator via llvm-commits]

Quuxplusone added inline comments.


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:254
 
+/// Finds the first instruction after both A and B.
+/// A and B are assumed to be either Instruction or Argument.
----------------
> This also does nothing to guarantee that all(or most) of the instructions will be removed.[...] Do we produce something as good as or better than what we would get if we left the user code alone?

> The only think left to do is to address the comment about other uses of intermediate values. The most restrictive approach would be to check all intermediate values if the number of uses matches the pattern?

IIUC, this is not a problem with _correctness_, right? We are protected against removing an instruction whose output still has live uses? But we're worried that the intermediate outputs will all have so many uses that we'll end up generating our MUL *and* keeping all those intermediate instructions, and so the codegen will be bigger than if we'd left it alone.

If I've understood the problem correctly, then I think @chfast's proposed solution is correct: you should do this optimization only if every intermediate result is completely dead (or can be replaced by a corresponding intermediate result of the new code). The vast majority of cases where we want this optimization to fire will be cases where all the intermediate results are dead.


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:279
+    if (U->getType() == FullTy && match(U, m_ZExt(m_Specific(X)))) {
+      for (const auto V : U->users()) {
+        if (match(V, m_c_Mul(m_Specific(U), m_ZExt(m_Specific(Y)))))
----------------
Here and lines 277 and 290: `const auto` should be something else (such as [simply `auto&&`](https://quuxplusone.github.io/blog/2018/12/15/autorefref-always-works/)), or else the `const` applies only to the copy you made of whatever the element type of `U->users()` was.  I suspect you actually meant `const auto *` but I'm not sure.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56214