This is an archive of the discontinued LLVM Phabricator instance.

Improve STRICT_FSETCC codegen in absence of no NaN
ClosedPublic

Authored by thopre on Nov 23 2020, 8:33 AM.

Details

Summary

As for SETCC, use a less expensive condition code when generating
STRICT_FSETCC if the node is known not to have Nan.

Diff Detail

Event Timeline

thopre created this revision.Nov 23 2020, 8:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2020, 8:33 AM
thopre requested review of this revision.Nov 23 2020, 8:33 AM
thopre updated this revision to Diff 307303.Nov 24 2020, 3:29 AM

Make FPMO const

SjoerdMeijer added inline comments.Feb 8 2021, 4:33 AM
llvm/test/CodeGen/AArch64/arm64-fcmp-no-nans-opt.ll
1

Since these tests run with -enable-no-nans-fp-math, I am wondering what condition we are testing:

if ((FPMO && FPMO->hasNoNaNs()) || TM.Options.NoNaNsFPMath)

I guess that is TM.Options.NoNaNsFPMath, so do we have/need checks for FPMO->hasNoNaNs()?

thopre updated this revision to Diff 322093.Feb 8 2021, 6:10 AM
thopre marked an inline comment as done.

Don't check for instruction nonan flag since there's none for constrained fcmp

Don't check for instruction nonan flag since there's none for constrained fcmp

But that check seems useful for float instructions? In other words, can we keep the check for FPMO->hasNoNaNs(), but just add some new tests for that?

thopre added inline comments.Feb 8 2021, 7:01 AM
llvm/test/CodeGen/AArch64/arm64-fcmp-no-nans-opt.ll
1

Ah yes, this only makes sense for regular fcmp which can have fast math flag nnan. Constrained fcmp cannot.

thopre added a comment.Feb 8 2021, 7:04 AM

Don't check for instruction nonan flag since there's none for constrained fcmp

But that check seems useful for float instructions? In other words, can we keep the check for FPMO->hasNoNaNs(), but just add some new tests for that?

Constrained FCmp instructions cannot be given fast-math flags in textual IR currently. I'm not sure if there's a way for the flag to be propagated from somewhere else. I'll try to dig a bit more. Bear with me

thopre updated this revision to Diff 322205.Feb 8 2021, 1:31 PM

Rename testcase

Don't check for instruction nonan flag since there's none for constrained fcmp

But that check seems useful for float instructions? In other words, can we keep the check for FPMO->hasNoNaNs(), but just add some new tests for that?

Constrained FCmp instructions cannot be given fast-math flags in textual IR currently. I'm not sure if there's a way for the flag to be propagated from somewhere else. I'll try to dig a bit more. Bear with me

Constrained fcmp can have such flags since any FPMathOperation can have the NoNaN flag but I could not find a way to set it from looking at the code. There are no parameter attribute that would achieve it, no easy way to express it with llvm.assume and no obvious combine that would work. Since there's no way to test that code I think it's safer to leave it out. Worst case it's an optimisation that does not happen and we can patch it later.

SjoerdMeijer accepted this revision.Feb 9 2021, 1:22 AM
SjoerdMeijer added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7015

Nit: perhaps just make this an if-else.

llvm/test/CodeGen/AArch64/arm64-constrained-fcmp-no-nans-opt.ll
3 ↗(On Diff #322205)

Do we need to add tests for .f16 and .f64 just for completeness?

This revision is now accepted and ready to land.Feb 9 2021, 1:22 AM
thopre marked an inline comment as done.Feb 9 2021, 2:45 AM
thopre added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7015

I'm not sure what you mean, something like:

ISD::CondCode Condition = getFcmpCondCode(FPCmp->getPredicate());
if (TM.Options.NoNaNsFPMath)
  Opers.push_back(DAG.getCondCode(getFCmpCodeWithoutNaN(Condition)));
else
  Opers.push_back(DAG>getCondCode(Condition));

? I'd prefer to keep the current code since it mirrors what is done in visitFcmp and reflect better that even if a better condition code can be used (due to no NaN) we are still going to push it. YMMV of course.

llvm/test/CodeGen/AArch64/arm64-constrained-fcmp-no-nans-opt.ll
3 ↗(On Diff #322205)

I've only done the f64 because f16 does not appear to be supported on AArch64:

LLVM ERROR: Cannot select: 0x55d983a0fc58: f16,ch = AArch64ISD::STRICT_FCMP 0x55d9839a8938, 0x55d983a0fab8, 0x55d983a0fb88
  0x55d983a0fab8: f16,ch = CopyFromReg 0x55d9839a8938, Register:f16 %0
    0x55d983a0fa50: f16 = Register %0
  0x55d983a0fb88: f16,ch = CopyFromReg 0x55d9839a8938, Register:f16 %1
    0x55d983a0fb20: f16 = Register %1
In function: f16_constrained_fcmp_ueq

Happy to add it later if someone adds that support.

thopre updated this revision to Diff 322324.Feb 9 2021, 2:46 AM
thopre marked an inline comment as done.

Add f64 tests

Add f64 tests

llvm/test/CodeGen/AArch64/arm64-constrained-fcmp-no-nans-opt.ll
3 ↗(On Diff #322205)

It is supported if you add -mattr=+fullfp16 to the command line.

SjoerdMeijer added inline comments.Feb 9 2021, 3:13 AM
llvm/test/CodeGen/AArch64/arm64-constrained-fcmp-no-nans-opt.ll
3 ↗(On Diff #322205)

And without it, it shouldn't crash, so that is a problem, but perhaps a different one.

thopre updated this revision to Diff 322327.Feb 9 2021, 3:14 AM
thopre marked 2 inline comments as done.

Add f16 cases

thopre added a comment.Feb 9 2021, 3:17 AM

LGTM

Great, thanks Sjoerd!

This revision was landed with ongoing or failed builds.Feb 9 2021, 3:18 AM
This revision was automatically updated to reflect the committed changes.
thopre added a comment.Feb 9 2021, 3:31 AM

LGTM

Great, thanks Sjoerd!

I had to revert it because the testcase was failing on the bot. I rebased the patch yesterday and it was passing locally with f16 and f64. Maybe a change between yesterday and today. I'll try to reproduce and reopen the diff if it needs changing.

thopre reopened this revision.Feb 9 2021, 7:31 AM
This revision is now accepted and ready to land.Feb 9 2021, 7:31 AM
thopre added a comment.Feb 9 2021, 7:33 AM

@SjoerdMeijer Ok to commit without f16 tests given the above? I've tested in debug mode and thus assertion on this time and that passes without issue (fails in that mode with f16 cases)

@SjoerdMeijer Ok to commit without f16 tests given the above? I've tested in debug mode and thus assertion on this time and that passes without issue (fails in that mode with f16 cases)

Ah, ok, sure.

Bonus points for following up to fix the f16 case then. ;-)

This revision was landed with ongoing or failed builds.Feb 11 2021, 6:19 AM
This revision was automatically updated to reflect the committed changes.