Page MenuHomePhabricator

[Reassociate] try harder to convert negative FP constants to positive
ClosedPublic

Authored by spatel on Aug 8 2019, 8:19 AM.

Details

Summary

This is an extension of a transform that tries to produce positive floating-point constants to improve canonicalization (and hopefully lead to more reassociation and CSE).

The original patches were:
D4904
D5363 (rL221721)

But as the test diffs show, these were limited to basic patterns by walking from an instruction to its single user rather than recursively moving up the def-use sequence. No fast-math is required here because we're only rearranging implicit FP negations in intermediate ops.

A motivating bug is:
https://bugs.llvm.org/show_bug.cgi?id=32939
...but unfortunately the transform in that example gets inverted in the backend, so we end up with the same code. I'll investigate that case as a follow-up, but I think the changes here can be viewed as an independent improvement.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Aug 8 2019, 8:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 8:19 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
spatel marked an inline comment as done.Aug 8 2019, 8:23 AM
spatel added inline comments.
llvm/test/Transforms/Reassociate/reassoc-intermediate-fnegs.ll
9–12 ↗(On Diff #214158)

I stepped through this case, and we don't remove the negative constant because ShouldBreakUpSubtract() is true. Even with the positive constant, we failed to factorize, so this needs work either way.

mcberg2017 added inline comments.Aug 8 2019, 9:08 AM
llvm/lib/Transforms/Scalar/Reassociate.cpp
2014 ↗(On Diff #214158)

How about in place of the assert, logic to bypass candidates that meet these constraints. It should not happen given how the candidate list is built, however this would ensure if something did get in we would not process it for release builds.

2021 ↗(On Diff #214158)

Same here...

I worked through some of the tests, but would like another set of eyes to verify. Doing a quick internal validation with this for regressions.

llvm/lib/Transforms/Scalar/Reassociate.cpp
2014 ↗(On Diff #214158)

Actually this is fine as it is...

We have some stir(some regressions - a few instructions here and there), however the change is overall beneficial for us.

LGTM.

spatel added a comment.Aug 8 2019, 1:25 PM

We have some stir(some regressions - a few instructions here and there), however the change is overall beneficial for us.

LGTM.

Thanks! If you can share any of those tests that regressed, I can dig further into this pass to see if we can improve them.

Tell you what, I don't want to hold up this patch. I will see if I can abstract a test example into common form for a public target with an example or two and send it to you offline.

mcberg2017 accepted this revision.Aug 9 2019, 11:39 AM
This revision is now accepted and ready to land.Aug 9 2019, 11:39 AM
This revision was automatically updated to reflect the committed changes.

Just to close the loop, the issues I saw are internal manifested only, they do not port to other targets.