llvm dev - Jan 2020 - Any significance for m_OneUse in (X / Y) / Z => X / (Y * Z) ??

A couple more general comments:
1. There shouldn't be any correctness issues removing one-use checks (the
transform should be safe independently of use-counts).
2. Ideally, you can remove a m_OneUse() from the code and run 'make
check'
or similar, and you will see a regression test failure because we have a
'negative' test to cover that pattern. That should make it clear that
the
one-use check is there intentionally and what the effect of removing it is.
We've gotten better about including those kinds of regression tests over
time, but I don't know what percentage of all instcombine transforms
actually have that test coverage.

On Mon, Dec 30, 2019 at 7:12 PM Craig Topper via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> As a general rule, InstCombine tries not increase the total number of
> instructions. If X/Y has another use other than this one, then it still
> ends up being calculated. Without the one use check you'd trade 2
fdivs,
> for 1 mul (Y * Z), and 2 fdivs ((X*Y)/Z) and the original (X / Y).
>
> ~Craig
>
>
> On Mon, Dec 30, 2019 at 4:07 PM raghesh via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> Dear All,
>>
>> The InstCombine pass performs the following transformation.
>>
>>    Z / (X / Y) => (Y * Z) / X
>>
>> This is performed only when operand Op1 ( (X/Y) in this case) has only
>> one use in future. The code snippet is shown below.
>>
>>     if (match(Op1, m_OneUse(m_FDiv(m_Value(X), m_Value(Y)))) &&
>>         (!isa<Constant>(Y) || !isa<Constant>(Op0))) {
>>       // Z / (X / Y) => (Y * Z) / X
>>       Value *YZ = Builder.CreateFMulFMF(Y, Op0, &I);
>>       return BinaryOperator::CreateFDivFMF(YZ, X, &I);
>>     }
>>
>> It would be great if someone explains if there is any issue
>> (correctness/performance-wise) if we avoid the m_OueUse check. What if
>> we perform the transformation even if there are multiple uses?
>>
>> There are similar transformations which perform the m_OueUse check. We
>> may avoid those too if there is no particular reason for the check.
>>
>> Regards,
>> ------------------------------
>> Raghesh Aloor
>> AMD India Pvt. Ltd.
>> Bengaluru.
>> ------------------------------
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200103/6a924184/attachment.html>

Thanks Sanjay for your comments.

So, if we want to add a transformation avoiding the one-use check, which
one is the ideal pass? Shall we do it in -aggressive-instcombine? I came to
know that if the pattern search complexity is O(n) [1] we should go for
aggressive-instcombine. If it is O(1) we must do that in -instcombine.
However, in my case, the complexity is still O(1) and want to avoid the
one-use check.

[1] n is the number of instructions in the function.

Regards,
------------------------------
Raghesh Aloor
AMD India Pvt. Ltd.
Bengaluru.
------------------------------


On Fri, Jan 3, 2020 at 8:02 PM Sanjay Patel <spatel at rotateright.com>
wrote:
> A couple more general comments:
> 1. There shouldn't be any correctness issues removing one-use checks
(the
> transform should be safe independently of use-counts).
> 2. Ideally, you can remove a m_OneUse() from the code and run 'make
check'
> or similar, and you will see a regression test failure because we have a
> 'negative' test to cover that pattern. That should make it clear
that the
> one-use check is there intentionally and what the effect of removing it is.
> We've gotten better about including those kinds of regression tests
over
> time, but I don't know what percentage of all instcombine transforms
> actually have that test coverage.
>
> On Mon, Dec 30, 2019 at 7:12 PM Craig Topper via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> As a general rule, InstCombine tries not increase the total number of
>> instructions. If X/Y has another use other than this one, then it still
>> ends up being calculated. Without the one use check you'd trade 2
fdivs,
>> for 1 mul (Y * Z), and 2 fdivs ((X*Y)/Z) and the original (X / Y).
>>
>> ~Craig
>>
>>
>> On Mon, Dec 30, 2019 at 4:07 PM raghesh via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> Dear All,
>>>
>>> The InstCombine pass performs the following transformation.
>>>
>>>    Z / (X / Y) => (Y * Z) / X
>>>
>>> This is performed only when operand Op1 ( (X/Y) in this case) has
only
>>> one use in future. The code snippet is shown below.
>>>
>>>     if (match(Op1, m_OneUse(m_FDiv(m_Value(X), m_Value(Y))))
&&
>>>         (!isa<Constant>(Y) || !isa<Constant>(Op0))) {
>>>       // Z / (X / Y) => (Y * Z) / X
>>>       Value *YZ = Builder.CreateFMulFMF(Y, Op0, &I);
>>>       return BinaryOperator::CreateFDivFMF(YZ, X, &I);
>>>     }
>>>
>>> It would be great if someone explains if there is any issue
>>> (correctness/performance-wise) if we avoid the m_OueUse check. What
if
>>> we perform the transformation even if there are multiple uses?
>>>
>>> There are similar transformations which perform the m_OueUse check.
We
>>> may avoid those too if there is no particular reason for the check.
>>>
>>> Regards,
>>> ------------------------------
>>> Raghesh Aloor
>>> AMD India Pvt. Ltd.
>>> Bengaluru.
>>> ------------------------------
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200106/461b4f3f/attachment-0001.html>

Is your case the case mentioned in the subject or a different case?

~Craig


On Sun, Jan 5, 2020 at 8:11 PM raghesh <raghesh.a at gmail.com> wrote:
> Thanks Sanjay for your comments.
>
> So, if we want to add a transformation avoiding the one-use check, which
> one is the ideal pass? Shall we do it in -aggressive-instcombine? I came
> to know that if the pattern search complexity is O(n) [1] we should go for
> aggressive-instcombine. If it is O(1) we must do that in -instcombine.
> However, in my case, the complexity is still O(1) and want to avoid the
> one-use check.
>
> [1] n is the number of instructions in the function.
>
> Regards,
> ------------------------------
> Raghesh Aloor
> AMD India Pvt. Ltd.
> Bengaluru.
> ------------------------------
>
>
> On Fri, Jan 3, 2020 at 8:02 PM Sanjay Patel <spatel at
rotateright.com>
> wrote:
>
>> A couple more general comments:
>> 1. There shouldn't be any correctness issues removing one-use
checks (the
>> transform should be safe independently of use-counts).
>> 2. Ideally, you can remove a m_OneUse() from the code and run 'make
>> check' or similar, and you will see a regression test failure
because we
>> have a 'negative' test to cover that pattern. That should make
it clear
>> that the one-use check is there intentionally and what the effect of
>> removing it is. We've gotten better about including those kinds of
>> regression tests over time, but I don't know what percentage of all
>> instcombine transforms actually have that test coverage.
>>
>> On Mon, Dec 30, 2019 at 7:12 PM Craig Topper via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> As a general rule, InstCombine tries not increase the total number
of
>>> instructions. If X/Y has another use other than this one, then it
still
>>> ends up being calculated. Without the one use check you'd trade
2 fdivs,
>>> for 1 mul (Y * Z), and 2 fdivs ((X*Y)/Z) and the original (X / Y).
>>>
>>> ~Craig
>>>
>>>
>>> On Mon, Dec 30, 2019 at 4:07 PM raghesh via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> Dear All,
>>>>
>>>> The InstCombine pass performs the following transformation.
>>>>
>>>>    Z / (X / Y) => (Y * Z) / X
>>>>
>>>> This is performed only when operand Op1 ( (X/Y) in this case)
has only
>>>> one use in future. The code snippet is shown below.
>>>>
>>>>     if (match(Op1, m_OneUse(m_FDiv(m_Value(X), m_Value(Y))))
&&
>>>>         (!isa<Constant>(Y) || !isa<Constant>(Op0)))
{
>>>>       // Z / (X / Y) => (Y * Z) / X
>>>>       Value *YZ = Builder.CreateFMulFMF(Y, Op0, &I);
>>>>       return BinaryOperator::CreateFDivFMF(YZ, X, &I);
>>>>     }
>>>>
>>>> It would be great if someone explains if there is any issue
>>>> (correctness/performance-wise) if we avoid the m_OueUse check.
What if
>>>> we perform the transformation even if there are multiple uses?
>>>>
>>>> There are similar transformations which perform the m_OueUse
check. We
>>>> may avoid those too if there is no particular reason for the
check.
>>>>
>>>> Regards,
>>>> ------------------------------
>>>> Raghesh Aloor
>>>> AMD India Pvt. Ltd.
>>>> Bengaluru.
>>>> ------------------------------
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200105/25a351d0/attachment.html>

If the transform is a performance optimization despite potentially
increasing the number of instructions, that should go in the backend. Most
likely that will be in DAGCombiner or a target-specific lowering file.

But as noted in the later reply, if this question is really about a
reciprocal special-case, that becomes an extension of what we already have
in instcombine.

To be clear - we're talking about FP math, and all of these transforms
require some form of fast-math-flags to be valid (no matter where they are
implemented).


On Sun, Jan 5, 2020 at 11:11 PM raghesh <raghesh.a at gmail.com> wrote:
> Thanks Sanjay for your comments.
>
> So, if we want to add a transformation avoiding the one-use check, which
> one is the ideal pass? Shall we do it in -aggressive-instcombine? I came
> to know that if the pattern search complexity is O(n) [1] we should go for
> aggressive-instcombine. If it is O(1) we must do that in -instcombine.
> However, in my case, the complexity is still O(1) and want to avoid the
> one-use check.
>
> [1] n is the number of instructions in the function.
>
> Regards,
> ------------------------------
> Raghesh Aloor
> AMD India Pvt. Ltd.
> Bengaluru.
> ------------------------------
>
>
> On Fri, Jan 3, 2020 at 8:02 PM Sanjay Patel <spatel at
rotateright.com>
> wrote:
>
>> A couple more general comments:
>> 1. There shouldn't be any correctness issues removing one-use
checks (the
>> transform should be safe independently of use-counts).
>> 2. Ideally, you can remove a m_OneUse() from the code and run 'make
>> check' or similar, and you will see a regression test failure
because we
>> have a 'negative' test to cover that pattern. That should make
it clear
>> that the one-use check is there intentionally and what the effect of
>> removing it is. We've gotten better about including those kinds of
>> regression tests over time, but I don't know what percentage of all
>> instcombine transforms actually have that test coverage.
>>
>> On Mon, Dec 30, 2019 at 7:12 PM Craig Topper via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> As a general rule, InstCombine tries not increase the total number
of
>>> instructions. If X/Y has another use other than this one, then it
still
>>> ends up being calculated. Without the one use check you'd trade
2 fdivs,
>>> for 1 mul (Y * Z), and 2 fdivs ((X*Y)/Z) and the original (X / Y).
>>>
>>> ~Craig
>>>
>>>
>>> On Mon, Dec 30, 2019 at 4:07 PM raghesh via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> Dear All,
>>>>
>>>> The InstCombine pass performs the following transformation.
>>>>
>>>>    Z / (X / Y) => (Y * Z) / X
>>>>
>>>> This is performed only when operand Op1 ( (X/Y) in this case)
has only
>>>> one use in future. The code snippet is shown below.
>>>>
>>>>     if (match(Op1, m_OneUse(m_FDiv(m_Value(X), m_Value(Y))))
&&
>>>>         (!isa<Constant>(Y) || !isa<Constant>(Op0)))
{
>>>>       // Z / (X / Y) => (Y * Z) / X
>>>>       Value *YZ = Builder.CreateFMulFMF(Y, Op0, &I);
>>>>       return BinaryOperator::CreateFDivFMF(YZ, X, &I);
>>>>     }
>>>>
>>>> It would be great if someone explains if there is any issue
>>>> (correctness/performance-wise) if we avoid the m_OueUse check.
What if
>>>> we perform the transformation even if there are multiple uses?
>>>>
>>>> There are similar transformations which perform the m_OueUse
check. We
>>>> may avoid those too if there is no particular reason for the
check.
>>>>
>>>> Regards,
>>>> ------------------------------
>>>> Raghesh Aloor
>>>> AMD India Pvt. Ltd.
>>>> Bengaluru.
>>>> ------------------------------
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200106/67a2a5b0/attachment-0001.html>