[PATCH] D86726: [InstCombine]: Transform 1.0/sqrt(X) * X to X/sqrt(X) and X * 1.0/sqrt(X) to X/sqrt(X)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added reviewers: grandinj, cameron.mcinally, lebedev.ri.
spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:552
+
+    // X * 1.0/sqrt(X) = X/sqrt(X).
+    if (match(Op1, (m_FDiv(m_SpecificFP(1.0), m_Value(Y)))) &&
----------------
grandinj wrote:
> venkataramanan.kumar.llvm wrote:
> > grandinj wrote:
> > > Just a drive-by commentator: I am surprised there is not an existing transform which does
> > >    1 * X ==> X
> > >    X * 1 ==> X
> > >    X / 1 ==> X
> > Yes they are available. 
> > 
> > This one is specifically targeting the pattern where one operand  for multiplication  is X and the other operand is Divide.  The dividend is 1.0 and divisor is sqrt(x).  
> > 
> > x * 1/sqrt(x) ==> x/sqrt(x)  later on we try to fold it to sqrt(x) under associative math option. 
> If they are available, then surely it is not necessary to do
> 
>     X * 1.0 / sqrt(X) ==> X/sqrt(X).
> 
> Surely by the time it gets there, that would already have been folded to
> 
>     X / sqrt(X)
> 
> ?
The motivation for this patch is not obvious because it is not stated in the summary, not shown in code comments, and the tests are not pre-committed to show diffs. Please do all 3 of those things.

This patch is a result of a decision made in D85709 (and I think that patch should be abandoned now): we are intentionally not folding x/sqrt(x) in IR because it loses information that the backend can't recover (for the case when x==0.0).

So the multi-use case of the 1/sqrt(x) factor is the only reason we need this specialized transform. We already handle more general cases, and we are intentionally not creating extra (and likely expensive) fdiv ops in IR. This pattern is the exception to the rule because we always expect the backend to reduce x/sqrt(x) if it has the necessary (reassoc) fast-math-flags.

Please see my comments in D86395 for an idea about how to write tests that provide coverage for the code proposed here (we need 2 tests to cover the 2 potential code patterns resulting from commutation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86726