This is an archive of the discontinued LLVM Phabricator instance.

[EarlyCSE] Retain poison flags, if program is UB if poison.
ClosedPublic

Authored by fhahn on Dec 7 2021, 5:52 AM.

Details

Summary

Poison-generating flags can be retained during CSE on the earlier
instruction , *if* the earlier instruction being poison causes UB.

Note that the code as written assumes that an instruction cannot have
poison-generating flags and fast-math flags, but I am not sure this is
always true. Should we add a version of andIRFlags that selectively
allows AND'ing fast-math flags only?

https://alive2.llvm.org/ce/z/4K3D7P

Diff Detail

Event Timeline

fhahn created this revision.Dec 7 2021, 5:52 AM
fhahn requested review of this revision.Dec 7 2021, 5:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2021, 5:52 AM
nikic added a comment.Dec 8 2021, 12:14 AM

Note that the code as written assumes that an instruction cannot have
poison-generating flags and fast-math flags, but I am not sure this is
always true. Should we add a version of andIRFlags that selectively
allows AND'ing fast-math flags only?

Yes, I think so. Or more specifically, to only and the non-poison-generating flags. There are some FMF that are poison generating, but we don't currently treat them as such in various "poison generating" queries. If that changed in the future, we should still handle it correctly.

fhahn updated this revision to Diff 393497.Dec 10 2021, 8:06 AM

Note that the code as written assumes that an instruction cannot have
poison-generating flags and fast-math flags, but I am not sure this is
always true. Should we add a version of andIRFlags that selectively
allows AND'ing fast-math flags only?

Yes, I think so. Or more specifically, to only and the non-poison-generating flags. There are some FMF that are poison generating, but we don't currently treat them as such in various "poison generating" queries. If that changed in the future, we should still handle it correctly.

I split out the code to 'and' non-poison generating flags in D115527 and update the patch to use it in case the program is not UB if I is poison.

Note that the code as written assumes that an instruction cannot have
poison-generating flags and fast-math flags, but I am not sure this is
always true. Should we add a version of andIRFlags that selectively
allows AND'ing fast-math flags only?

Yes, I think so. Or more specifically, to only and the non-poison-generating flags. There are some FMF that are poison generating, but we don't currently treat them as such in various "poison generating" queries. If that changed in the future, we should still handle it correctly.

We have a few poison-related patches in progress currently, so there may be some interaction between these:
D115460 would add ninf/nnan to the list of poison-generating flags.
D115526 would remove a demanded elements restriction for propagation of flags

reames requested changes to this revision.Dec 10 2021, 8:26 AM

Conceptually this is clear enough, but I'm worried about compile time impact. Do you have any numbers to share? This looks potentially quite expensive as the analysis to prove UB if poison is not really cacheable.

We can improve compile time by e.g. only doing the search on things which might produce poison flags, but the first step is knowing if we have a problem.

llvm/lib/Transforms/Scalar/EarlyCSE.cpp
1369–1379

Per LangRef, nnan and ninf are poison generating. The handling on the other FMF flags is unclear to me. I'd suggest as a stepping stone restricting the preservation of flags to non-float instructions while leaving a clear TODO.

This revision now requires changes to proceed.Dec 10 2021, 8:26 AM
fhahn updated this revision to Diff 393563.Dec 10 2021, 11:56 AM

Updated again to be independet of D115527 again, always use 'and' of flags for floating point insts.

We have a few poison-related patches in progress currently, so there may be some interaction between these:
D115460 would add ninf/nnan to the list of poison-generating flags.
D115526 would remove a demanded elements restriction for propagation of flags

Thanks for those references. I think D115460 would impact D115527, but not this patch directly. I went ahead and applied Philip's suggestion to always take the 'and' for floating point instructions.

Conceptually this is clear enough, but I'm worried about compile time impact. Do you have any numbers to share? This looks potentially quite expensive as the analysis to prove UB if poison is not really cacheable.

We can improve compile time by e.g. only doing the search on things which might produce poison flags, but the first step is knowing if we have a problem.

Compile-time looks neutral when limiting the UB check to instructions with poison-generating flags, as in the original version, with the highest geoman increase of +0.04%. Note that SPASS for NewPM-ReleaseThinLTO has an increase of +0.17% and is highlighted red. For that workload, there's also a +0.16% size increase, so the increase is likely from additional transformations.

http://llvm-compile-time-tracker.com/compare.php?from=2a31b240df1ce1724960fd7cf98f673064b44206&to=e3d5cc5e3011f38eb95dc60f4a4762a40a7031a7&stat=instructions

fhahn marked an inline comment as done.Dec 10 2021, 11:58 AM
fhahn added inline comments.
llvm/lib/Transforms/Scalar/EarlyCSE.cpp
1369–1379

Update and added todo. D115527 still seems helpful, but probably only once the poison-generating FMFs are explicitly handled.

nikic added inline comments.Dec 10 2021, 11:59 AM
llvm/lib/Transforms/Scalar/EarlyCSE.cpp
1376

This should be checking isa<FPMathOperator> instead. It's not necessarily the result type that's a float (e.g. fcmp).

fhahn updated this revision to Diff 393567.Dec 10 2021, 12:21 PM
fhahn marked an inline comment as done.

Check for FPMathOperator.

fhahn marked an inline comment as done.Dec 10 2021, 12:21 PM
fhahn added inline comments.
llvm/lib/Transforms/Scalar/EarlyCSE.cpp
1376

fixed, thanks!

nikic accepted this revision.Dec 10 2021, 12:25 PM

LG

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

I think this would be more obvious as isa<FPMathOperator>(I) || (I->hasPoisonGeneratingFlags() && !programUndefindeIfPoison(I)), though in practice it's the same.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 11 2021, 7:12 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
fhahn marked an inline comment as done.