I applaud the change, but i'm probably not the right reviewer for this.

@ reviewers - i'm not so much interested in deep code/algo review,
but more like in the general direction disscussion, like, is this okay for instcombine? :)

(marking as reviewed)

As per @asbirlea's comments.

Not needed unless D84609 proceeds.

Apologies if i'm missing the point here.

One last tiny cleanup.

In D82847#2211668, @tstellar wrote:

Reverting this patch seems to fix the error for me. Are you still seeing this same error in trunk?

In D82847#2211661, @tstellar wrote:

This commit appears to be causing an error when I run CMake:

CMake Error:

Running

 '/usr/bin/ninja-build' '-C' '/home/tstellar/llvm-project/llvm-build' '-t' 'cleandead'

failed with:

 ninja: error: remove(include/llvm/Support): Directory not empty

CMake Generate step failed. Build files cannot be regenerated correctly.

LGTM, thank you!

In D85709#2209979, @cameron.mcinally wrote:

I'm fairly sure this transform is a performance loss. For a target like Skylake Server, a SQRT(x) can take up to 20 cycles. But a RSQRT(x) is about 6 cycles and a MUL(y) is 4 cycles. We'd be better off with a X*RSQRT(X).

Test?

Passing-by remark: This change passed in our internal huge code base. isn't a great expanded description for the change..

In D85446#2205616, @bjope wrote:

(Just some heads up, in case someone else got the same problem...)

I've noticed some regressions downstream after this patch. Haven't debugged/reduced it completely yet, but at least in one case I suspect that the problem actually might be in load-store-vectorizer.

Used to get something like this before load-store-vectorizer:

<...>

The old input to load-store-vectorizer resulted in two <8 x i16> loads, but the new input gives one <8 x i16 load, two <2 x i16> loads and one <4 x i16> load. So for some reason LSV fails to detect that the loads are consecutive given the new IR. At least that is my current theory.

Let's not introduce any new unneeded one-use checks.

I approve of the general direction of this patch series :)
Some high-level notes. I'm not that familiar with the code in question,
so it will be best for someone else to review, too.

@efriedma sudo-reverse-ping, thanks.

Hm, guess i missed this pattern :/

I feel like this is pretty identical to the D85046, so lgtm modulo some possible generalization.

I'm not sure why i'm added as a reviewer on this patch, i don't work on ppc, i never used it, and i never really worked on that pass.

(can you please maybe spellcheck both the patch description and the wording within the patch? it is really hard to read)

Aha, good catch.
But this is not enough of a generalization:

diff
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
index 172b304b776..06a0fcbf0fa 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
@@ -233,29 +233,11 @@ Instruction *InstCombinerImpl::visitMul(BinaryOperator &I) {
     }
   }

If that's what's works for BPF..

Abandon this? D66324 landed.

LGTM, but i would have preferred to see more feedback on the RFC thread.
That being said, unless you are planning on forming calls to these intrinsics in middle-end transform passes
(i.e. only planning on using them in clang codegen), i think this is okay to proceed.

@kuhar thank you for the review!
@yrouban thank you for the fix!

Abandon now that D84763 has resolved the base problem?

Abandon now that D84763 has resolved the base problem?

Abandon now that D84763 has resolved the base problem?

Abandon now that D84763 has resolved the base problem?

Glad to see that D84763 is making it's way!

Still no replies to RFC thread :/

Looks good to me.

lg

Ignoring the question of direction, the code change itself seems ok to me.

I'm not really familiar with code in question, but ignoring
the question of whether or not this is moving in the right
direction, the code change itself looks like a cleanup to me.

@xldenis if you need help committing do state so, and specify the Author: name <e@ma.il> line for commit attribution.

Not sure this needs review.

In D74436#2190492, @lebedev.ri wrote:

<...>

In D74436#2190422, @fhahn wrote:

IIUC the patch is currently reverted (in 78654e8511cf16d49f6680d782f3771a767ba942), due to ~20 llvm-test-suite failures.

The failures on X86 are listed below. Unfortunately most tests just compare the hash of the output, so it is not obvious in what way the results are changed.

Presumably it means that whatever optimization options those test specify, are now either more strict or less strict.
I think this is really missing some codegen tests that show how codegen actually changes by this.