Page MenuHomePhabricator

[ConstantFolding] Fold constrained compare intrinsics
Needs ReviewPublic

Authored by sepavloff on Sep 23 2021, 5:03 AM.

Details

Summary

The change implements constant folding of ‘llvm.experimental.constrained.fcmp’
and ‘llvm.experimental.constrained.fcmps’ intrinsics.

Diff Detail

Event Timeline

sepavloff created this revision.Sep 23 2021, 5:03 AM
sepavloff requested review of this revision.Sep 23 2021, 5:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2021, 5:03 AM
bjope resigned from this revision.Sep 23 2021, 5:53 AM

Sorry, I don't know much about fcmp.

sepavloff updated this revision to Diff 374813.Sep 24 2021, 5:36 AM

Add check lines to the test

kpn added inline comments.Sep 27 2021, 11:00 AM
llvm/lib/Analysis/ConstantFolding.cpp
1853

This still worries me. Are _analysis_ passes allowed to change the input IR? What if the caller decides to not do a fold after calling this function to see if a fold is possible? And why is the ebIgnore case different from the other two?

Constant folding won't add this ReadNone attribute when the constant folding code doesn't have an Instruction to alter. I don't know what to do about that.

llvm/lib/IR/Instructions.cpp
4134

Besides moving this code to a new function, are there any changes to it? It's hard to tell.

llvm/test/Transforms/InstSimplify/constfold-constrained.ll
418

There are no "maytrap" tests here. For the sake of future readers it would probably be useful to show that "maytrap" was taken into account and is working correctly.

Added tests with "maytrap"

nikic added a subscriber: nikic.Sep 30 2021, 12:20 PM
nikic added inline comments.
llvm/lib/Analysis/ConstantFolding.cpp
1853

Analysis passes are indeed not allowed to change the IR. And this isn't a harmless change either if it gets left behind -- e.g. it invalidates MemorySSA.

sepavloff added inline comments.Sep 30 2021, 11:31 PM
llvm/lib/Analysis/ConstantFolding.cpp
1853

Analysis passes are indeed not allowed to change the IR.

Constant folding is not an analysis, it changes IR. I don't know why this file is in Analysis directory.

What if the caller decides to not do a fold after calling this function to see if a fold is possible?

Nothing bad would happen. Only side effect would be removed, but we know for sure that it is absent, we just evaluated the operation. The function call would not be removed if its result is used.

And why is the ebIgnore case different from the other two?

If exception behavior is ebIgnore, such calls will get attribute SDNodeFlags::NoFPExcept in DAG and such instructions do not have side effects. But setting ReadNone for instructions with ebIgnore allows removal of these instructions earlier, at IR level, which could have positive effect.

And this isn't a harmless change either if it gets left behind -- e.g. it invalidates MemorySSA.

Comparison of two floating point numbers do not use memory access. But it can change bits in the floating point state register (only Invalid bit can be set). This change is emulated as memory access so that instruction be ordered correctly. This is why constrained intrinsics declared with attribute IntrInaccessibleMemOnly. As no actual memory access occurs, it is harmless to set ReadNone in this case.

llvm/lib/IR/Instructions.cpp
4134

No, this is only a code moving.

llvm/test/Transforms/InstSimplify/constfold-constrained.ll
418

Added tests with "maytrap".

Set ReadNone for all intrinsics if they can be evaluated without raising signals

nikic added a subscriber: lebedev.ri.
nikic added inline comments.
llvm/lib/Analysis/ConstantFolding.cpp
1853

Constant folding is not an analysis, it changes IR. I don't know why this file is in Analysis directory.

The constant folding analysis does not change IR. Users of the constant folding analysis change IR based on the analysis result.

Comparison of two floating point numbers do not use memory access. But it can change bits in the floating point state register (only Invalid bit can be set). This change is emulated as memory access so that instruction be ordered correctly. This is why constrained intrinsics declared with attribute IntrInaccessibleMemOnly. As no actual memory access occurs, it is harmless to set ReadNone in this case.

My point here was that adding a readnone attribute invalidates MemorySSA, because it means that the instruction should no longer have a MemoryAccess -- it would result in a verification failure. Just calling ConstFold/InstSimplify should never have this kind of effect.

Unless @spatel or @lebedev.ri tell me I'm wrong here, I believe this should be considered a blocker for further work in this area.

lebedev.ri requested changes to this revision.Oct 2 2021, 9:36 AM

Unless @spatel or @lebedev.ri tell me I'm wrong here, I believe this should be considered a blocker for further work in this area.

This revision now requires changes to proceed.Oct 2 2021, 9:36 AM
sepavloff updated this revision to Diff 388475.Fri, Nov 19, 5:22 AM

Move attribute modification to InstSimplify pass