This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombine]: Fold X/Sqrt(X) to Sqrt(X).
ClosedPublic

Authored by venkataramanan.kumar.llvm on Aug 22 2020, 10:49 AM.

Details

Summary

Under -ffast-math ( "nnan" and " reassoc") fold X/Sqrt(X) to Sqrt(X).

Diff Detail

Event Timeline

venkataramanan.kumar.llvm requested review of this revision.Aug 22 2020, 10:49 AM

Hi Sanjay,

As discussed in InstCombine patch, moving the transformation to generic "DagCombine". I have adjusted the test case for X86_64 target. I am not sure if I need to do add/change test cases for other targets.

Hi Sanjay,

As discussed in InstCombine patch, moving the transformation to generic "DagCombine". I have adjusted the test case for X86_64 target. I am not sure if I need to do add/change test cases for other targets.

Yes, you need to update any tests that are no longer passing with this patch. That's why the buildbot flagged this patch with failures.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13360

I don't think 'nnan' is required. A NAN input results in NAN output either way.
NAN/sqrt(NAN) == NAN == sqrt(NAN)

I included 'arcp' in the tests that I added just to make sure that we were bypassing the sqrt estimate creation.

spatel added inline comments.Aug 22 2020, 1:15 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13361

Also, 'reassoc' generally means anything can happen, but if we want to be safer, we could require 'nsz' because this transform would change the result for a -0.0 input.

Updated the patch as per the comments given by Sanjay. Adjusted the test cases.

spatel accepted this revision.Aug 23 2020, 6:10 AM

See inline for a couple of minor points, otherwise LGTM.
Adding potential reviewers from the earlier patch review to see if anyone else has comments.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13362–13364

Formatting nit: the LLVM coding standards are flexible about when to use braces, but the choice here seems to not have any advantages. It would be better to either (1) omit braces entirely or (2) use braces on the first 'if' only.

llvm/test/CodeGen/AArch64/sqrt-fastmath.ll
494

In general, I prefer to make test changes in a preliminary commit, so there's no question about the functional difference created by the change in the compiler code. I can do that on your behalf if you do not have commit privileges.

This revision is now accepted and ready to land.Aug 23 2020, 6:10 AM
llvm/test/CodeGen/AArch64/sqrt-fastmath.ll
494

Sure please do on behalf me.

Updated patch as per the comments from Sanjay.