Page MenuHomePhabricator

AMDGPU/GlobalISel Check for NoNaNsFPMath in isKnownNeverSNaN
ClosedPublic

Authored by Petar.Avramovic on Thu, Sep 10, 7:56 AM.

Details

Summary

Check for NoNaNsFPMath function attribute in isKnownNeverSNaN.
Function attributes are in held in 'TargetMachine.Options'.
Among other things, this allows selection of some patterns imported
in D87351 since G_FCANONICALIZE is not generated when isKnownNeverSNaN
returns true in lowerFMinNumMaxNum.

However we notice some incorrect results since function attributes are
not correctly written in TargetMachine.Options when next function is
processed. Look at @v_test_no_global_nnans_med3_f32_pat0_srcmod0, it has
"no-nans-fp-math"="false" but TargetMachine.Options still has it set to
true since first function in test file had this attribute set to true.
This will be fixed by in D87511.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptThu, Sep 10, 7:56 AM
Petar.Avramovic requested review of this revision.Thu, Sep 10, 7:56 AM

With G_FCANONICALIZE out of the way, we can now select some of the med3 patterns that were imported in D87351.

llvm/test/CodeGen/AMDGPU/GlobalISel/fmed3.ll
70–72

This was not folded because G_FNEG was expected.
TODO: add combine for G_FNEG.

Probably should add a dedicated test for this

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
2658 ↗(On Diff #290976)

Should copy the comment from the DAG

// Reset the target options before resetting the optimization
// level below.
// FIXME: This is a horrible hack and should be processed via
// codegen looking at the optimization level explicitly when
// it wants to look at it.
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
5420 ↗(On Diff #290976)

I think checking these fields should be buried inside isKnownNeverSNaN

Petar.Avramovic retitled this revision from AMDGPU/GlobalISel Check target options before isKnownNeverSNaN to AMDGPU/GlobalISel Check for NoNaNsFPMath in isKnownNeverSNaN.
Petar.Avramovic edited the summary of this revision. (Show Details)

Move change in IRTranslator in another patch, since effects of that change can only be seen when function attributes are inspected during translation of a function that was not first in input file.
Move check for NoNaNsFPMath in isKnownNeverSNaN.
Reduce number of tests, only keep characteristic ones that will be updated when we add some combines and improvements to legalizer. Later switch to existing test in sdag that also checks for pattern commutes.

foad added a comment.Fri, Sep 11, 6:50 AM

This will be fixed by in D87511.

Wouldn't it make more sense to change the order of the patches in the stack so that D87511 is applied first and this one is rebaed on top of it?

This will be fixed by in D87511.

Wouldn't it make more sense to change the order of the patches in the stack so that D87511 is applied first and this one is rebaed on top of it?

Yes, but it is this way because of the regression test. Change is visible in output when there is something that accesses function attributes.

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
2658 ↗(On Diff #290976)

This does not seem relevant since global-isel does not reset opt level.

arsenm accepted this revision.Fri, Sep 11, 2:05 PM
arsenm added inline comments.
llvm/lib/CodeGen/GlobalISel/Utils.cpp
471–472

Should delete this null check in a separate change

474–478

Could merge these into getFlag() || TM.Options ...

This revision is now accepted and ready to land.Fri, Sep 11, 2:05 PM