This is an archive of the discontinued LLVM Phabricator instance.

[SLC] Expand the simplification of pow(x, 0.5) to sqrt(x)

Authored by evandro on Jul 30 2018, 6:59 PM.



Expand the number of cases when pow(x, 0.5) is simplified into sqrt(x) by considering the math semantics with more granularity.

Diff Detail


Event Timeline

evandro created this revision.Jul 30 2018, 6:59 PM
evandro updated this revision to Diff 158437.Jul 31 2018, 6:11 PM
evandro edited the summary of this revision. (Show Details)

Ping! ๐Ÿ””

spatel added inline comments.Aug 14 2018, 6:27 AM
8โ€“9 โ†—(On Diff #158437)

Why change the data types?

20 โ†—(On Diff #158437)

The minimal test for this case requires no FMF?

30 โ†—(On Diff #158437)

Why change the data types? Same question for the later tests - the types should be irrelevant to the change in functionality, right?

Please make any IR changes to the tests that are necessary to show off the improved/fixed functionality as a preliminary step to this review.

I'll commit the data type changes increasing type diversity on the side.

8โ€“9 โ†—(On Diff #158437)

In order to increase test coverage. Otherwise, it virtually tested for double only.

20 โ†—(On Diff #158437)

IIUC, not all of them, just AFN.

30 โ†—(On Diff #158437)

Replacing AFN for NINF shows the improved functionality.

lebedev.ri added inline comments.
8โ€“9 โ†—(On Diff #158437)

Tests are cheap. Couldn't you just *add* the additional test coverage with floats?
Changing existing tests is not always the best way..

evandro added inline comments.Aug 14 2018, 8:56 AM
8โ€“9 โ†—(On Diff #158437)

Yes, you're right.

spatel added a subscriber: scanon.Aug 14 2018, 9:11 AM
spatel added inline comments.
20 โ†—(On Diff #158437)

'afn' means this transform can diverge from the original code in some way, but I don't think that's true here. Ie, sqrt(x) should be the same as pow(x, 0.5) for all values other than -0.0 and -INFINITY.
Not sure if that is specified by any standard though? (cc @scanon)

evandro added inline comments.Aug 14 2018, 9:28 AM
20 โ†—(On Diff #158437)

The test originally had AFN, so I aimed at preserving it.

spatel added inline comments.Aug 14 2018, 12:17 PM
20 โ†—(On Diff #158437)

It's not obvious where the holes are in the existing code, so I added tests at:
rL339713 (fix copy/paste errors)

If you want to vary the data types more in those tests, feel free. But I think that should provide coverage for everything that is changing in this patch based on FMF + libcall vs. intrinsic. Please rebase this patch with those tests.

evandro updated this revision to Diff 160875.Aug 15 2018, 11:45 AM
evandro marked 11 inline comments as done.
spatel accepted this revision.Aug 15 2018, 12:22 PM


This revision is now accepted and ready to land.Aug 15 2018, 12:22 PM
This revision was automatically updated to reflect the committed changes.
evandro added a subscriber: fhahn.Aug 17 2018, 8:01 AM