This is an archive of the discontinued LLVM Phabricator instance.

Fix floating point math function attributes definition.
ClosedPublic

Authored by michele.scandale on May 22 2020, 5:33 PM.

Details

Reviewers
rjmccall
mibintc
Summary

With the support for changing the state of floating point optimizations
within a code block, we need to make sure that the function attributes
related to floating point math optimization are set in a conservative
way w.r.t. the body of the function to prevent illegal transformations
in the backends where such attributes are consumed.

Diff Detail

Unit TestsFailed

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 22 2020, 5:33 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I'll review this when it's rebased on top of the other commit.

michele.scandale edited the summary of this revision. (Show Details)Jun 1 2020, 9:03 PM

Can we do this conservatively only if something is changed within the function definition, and otherwise respect the global settings?

Can we do this conservatively only if something is changed within the function definition, and otherwise respect the global settings?

I'm not sure I understand what you mean. The initial state is already the global settings coming from the language options (see constructor of CodeGenFunction).
Then whenever we process a unary/binary floating point operator we consider the local FPOptions and adjust the values of the function attributes accordingly.

Maybe instead of having an explicit state (the booleans) in CodeGenFunction tracking the values for these function attributes, I could simply update the function attributes directly. I guess the extra cost is negligible.

We have a very similar problem with the constrained FP intrinsics where the use of the intrinsics anywhere in a function requires (1) an attribute to be set on the function and (2) all the other code in the function to be emitted with intrinsics. What I've been treating as the plan of record is that we'll handle this by setting an attribute on the (AST) function definition whenever we see a relevant local pragma within it. And if we're doing that anyway, I think we can use it for this as well: set them in getDefaultFunctionAttributes and then clear them off if we see the attribute.

As for the RAII objects, my biggest concern here is the amount of extra work every time we emit an FP expression when we should have already set up the default configuration correctly. In the recent refactor, we started only storing FPOptions when there are non-default options for an expression. We should be able to use that to stop retranslating FPOptions to LLVM's options on every expression. (We can consider optimizing it for consecutive operations under the same non-default settings later if it proves important.)

Modify IR function attribute directly.
RAII object modifies IR builder state and function attributes only if needed.
Rebase.

Store CGF in RAII object.

rjmccall accepted this revision.Jun 8 2020, 8:13 PM

This looks great, thanks.

This revision is now accepted and ready to land.Jun 8 2020, 8:13 PM

As far as I can tell the failures reported shouldn't be caused by this change.

John, would you be able to land this on my behalf?

rjmccall closed this revision.Jun 11 2020, 3:17 PM

To ssh://github.com/llvm/llvm-project

a98d618f6e5f..7fac1acc6171  master -> master