Page MenuHomePhabricator

[FPEnv][EarlyCSE] Add support for CSE of constrained FP intrinsics, take 2
ClosedPublic

Authored by kpn on Oct 21 2021, 11:50 AM.

Details

Summary

Currently we only CSE constrained FP intrinsics in the default floating point environment.

This patch adds support for CSE when exception behavior is "ignore" or "maytrap" and the rounding mode is known.

This review replaces D105895. No restrictions on CSE across function calls is in this patch.

Diff Detail

Event Timeline

kpn requested review of this revision.Oct 21 2021, 11:50 AM
kpn created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2021, 11:50 AM
kpn added a reviewer: bkramer.Apr 5 2022, 10:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 10:40 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
kpn updated this revision to Diff 420575.Apr 5 2022, 10:42 AM

Rebase.

kpn updated this revision to Diff 434453.Jun 6 2022, 6:19 AM

Rebase.

spatel added a comment.Jul 6 2022, 5:47 AM

I could use a high-level refresher (more info in the patch description) - why do we need to defer replacement on constrained intrinsics? The answer might be visible in that last set of tests, but I am not sure.

llvm/lib/Transforms/Scalar/EarlyCSE.cpp
1387

Is it possible that the optional ExceptionBehavior was not present?

1387–1388

Add another assert that RoundingMode (if present) is not Dynamic?

1422–1423

formatting

1623

formatting

llvm/test/Transforms/EarlyCSE/tfpropagation.ll
5

I'm not sure what changes we want to see in this set of tests - please add a comment like the other files have.

Adding some more potential reviewers. I haven't gone far enough into CSE to see corner-cases on something like this.

kpn updated this revision to Diff 447448.Mon, Jul 25, 1:22 PM

You know, I now believe that I can simplify this down to just this. The complication to treat constrained intrinsics as a memory update was put in when I added the restrictions on doing CSE across function calls. But since that restriction isn't needed I don't see the need for the rest of the complications. I don't think we need to go to heroics just for exception handlers, at least not at this time. This code is now much simpler.

You know, I now believe that I can simplify this down to just this. The complication to treat constrained intrinsics as a memory update was put in when I added the restrictions on doing CSE across function calls. But since that restriction isn't needed I don't see the need for the rest of the complications. I don't think we need to go to heroics just for exception handlers, at least not at this time. This code is now much simpler.

That is a lot neater. :)
I'm still not sure what you're trying to show in that last test file. It should be fine to pre-commit the tests with baseline results now (that might help highlight a diff?).

kpn updated this revision to Diff 448360.Thu, Jul 28, 9:13 AM

Update diff after precommit of tests.

spatel added inline comments.Sun, Jul 31, 12:25 PM
llvm/test/Transforms/EarlyCSE/tfpropagation.ll
125

Am I seeing this correctly - all tests now transform?
Should there be tests where one or both of the constrained ops is "fpexcept.strict"?
Please add some explanatory comments for these tests.

kpn added a comment.Mon, Aug 1, 6:15 AM

I'll update the patch later today with that missing assertion.

llvm/lib/Transforms/Scalar/EarlyCSE.cpp
1387

No. I believe all constrained intrinsics specify an exception behavior.

1387–1388

Will do.

llvm/test/Transforms/EarlyCSE/tfpropagation.ll
125

I precommitted the tests. The tests that aren't showing up in the diff now were the negative tests that included "fpexcept.strict" and the dynamic rounding tests. Since those test for changes that must not happen, this patch here shows no changes to those tests.

What sort of explanatory comments do you have in mind?

spatel added inline comments.Mon, Aug 1, 6:26 AM
llvm/test/Transforms/EarlyCSE/tfpropagation.ll
125

I'm still not seeing it - are the tests with fpexcept.strict not in this file?
It's not clear to me what conditions are needed to propagate the true/false - is it the same as the other transforms (must not be fpexcept.strict and must not be dynamic rounding)?

kpn added a comment.Mon, Aug 1, 6:33 AM

I'll get back to you on the tfpropagation.ll question. The test precommit:

commit 25a83005ef9dfce7c40c295a987f9e0ad7dc76d3 (origin/main, main)
Author: Kevin P. Neal <kevin.neal@sas.com>
Date: Wed Jul 27 15:21:17 2022 -0400

Precommit tests for D112256 "[FPEnv][EarlyCSE] Add support for CSE of constrained FP intrinsics, take 2"
kpn updated this revision to Diff 450314.Fri, Aug 5, 9:56 AM

Address review comments.

I'm leaving support for strict exception behavior out because it would make the patch more complicated and I'm really not sure the change would be worth it. Since we can't eliminate any traps with ebStrict we can't really CSE. So the added complication would only benefit this propagation of constant true and false. And said complication wouldn't have anything to do with the "maytrap" case that this ticket is for anyway.

Address review comments.

I'm leaving support for strict exception behavior out because it would make the patch more complicated and I'm really not sure the change would be worth it. Since we can't eliminate any traps with ebStrict we can't really CSE. So the added complication would only benefit this propagation of constant true and false. And said complication wouldn't have anything to do with the "maytrap" case that this ticket is for anyway.

My previous comments might've been unclear: I'm not asking for a functional change, I'm just wondering why there is no test for strict in the true/false test file. Shouldn't it be there for completeness independently of this patch?

kpn updated this revision to Diff 452270.Fri, Aug 12, 12:29 PM

I was thinking that adding a test for strict exception behavior might make it look intentional that this true/false propagation isn't happening. But a test case with a note is perfectly fine.

spatel accepted this revision.Mon, Aug 15, 4:58 AM

LGTM

This revision is now accepted and ready to land.Mon, Aug 15, 4:58 AM
This revision was landed with ongoing or failed builds.Tue, Aug 16, 5:31 AM
This revision was automatically updated to reflect the committed changes.