Page MenuHomePhabricator

Fix denormal-fp-math flag and attribute interaction
ClosedPublic

Authored by arsenm on Dec 11 2019, 6:34 AM.

Details

Summary

Make these behave the same way unsafe-fp-math and co. The command line
flag should add the attribute to functions that do not already have
it, and leave existing attributes. The attribute is the actual
implementation, but the flag is useful in some testing situations.

AMDGPU has a variety of tests with denormals enabled/disabled that
would require a painful level of test duplication without a flag. This
doesn't expose setting the separate input/output modes, or add a flag
for the f32 version yet.

Tests will be included in future patch.

Diff Detail

Event Timeline

arsenm created this revision.Dec 11 2019, 6:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2019, 6:34 AM
arsenm updated this revision to Diff 249168.Mar 9 2020, 11:19 AM

Rebase and fix patch split

This is wanting a command line flag rather than setting it on the function, or rather in addition to being able to set it on the function? Also "this" in the commit message isn't quite clear. Having this be a separate set of both global function attributes and different than the other fp-math attributes means it really should be called out fairly explicitly why they need to be different, what the semantics are, etc.

arsenm edited the summary of this revision. (Show Details)Mar 9 2020, 7:46 PM

This is wanting a command line flag rather than setting it on the function, or rather in addition to being able to set it on the function? Also "this" in the commit message isn't quite clear. Having this be a separate set of both global function attributes and different than the other fp-math attributes means it really should be called out fairly explicitly why they need to be different, what the semantics are, etc.

This makes the denormal handling consistent with the fp-math attributes. We previously had inconsistent behaviors, but now the flags should never override an existing attribute

So I'm still concerned that this is on TargetOptions at all. Ideally for new attributes like this we'd only put them on the IR - at the way you've got it we're making resetTargetOptions even harder to remove. I don't think this is the right path forward (as removing resetTargetOptions is the right path) and this is only making that harder.

arsenm updated this revision to Diff 250106.Thu, Mar 12, 5:31 PM

Drop parts threading through TargetOptiions like the other flags

echristo accepted this revision.Fri, Mar 27, 12:15 PM

Sure. I'd prefer we remove it from the struct completely, but that can happen later.

This revision is now accepted and ready to land.Fri, Mar 27, 12:15 PM