This is an archive of the discontinued LLVM Phabricator instance.

Add support for STRICT_FSETCC promotion
ClosedPublic

Authored by thopre on Nov 23 2020, 6:13 AM.

Details

Summary

Add missing handling of STRICT_FSETCC promotion. This prevents assert
failure in llvm::TargetLoweringBase::getTypeToPromoteTo().

Diff Detail

Event Timeline

thopre created this revision.Nov 23 2020, 6:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2020, 6:13 AM
thopre requested review of this revision.Nov 23 2020, 6:13 AM
uweigand requested changes to this revision.Nov 23 2020, 10:23 AM
uweigand added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
4619

When expanding a strict FP operation, we need to take care to never emit any non-strict operation. It looks like the patch as currently written might cause a "FP_EXTEND" to be emitted, which would violate that rule.

We need to emit a STRICT_FP_EXTEND instead (which needs to be properly chained just before the new STRICT_FSETCC node). Then everything should be OK since the only case where STRICT_FP_EXTEND might raise an exception is if the input is a (signaling) NaN, in which case the original STRICT_FSETCC would also have raised an exception.

This revision now requires changes to proceed.Nov 23 2020, 10:23 AM

Oh, any another thing: we might also want to handle STRICT_FSETCCS in addition to STRICT_FSETCC.

thopre updated this revision to Diff 307295.Nov 24 2020, 3:05 AM
thopre marked an inline comment as done.

Use STRICT_FP_EXTEND for strict FP and also add support for STRICT_FSETCCS

See the two inline comments about STRICT_FSETCCS. I guess that shows this path is not really exercised right now -- maybe it would be a good idea to add some tests.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
4421

I guess this needs to handle STRICT_FSETCCS here as well.

4633

Should use STRICT_FSETCCS here if we're expanding STRICT_FSETCCS.

See the two inline comments about STRICT_FSETCCS. I guess that shows this path is not really exercised right now -- maybe it would be a good idea to add some tests.

The problem is that no target currently promote a STRICT_FSETCC or STRICT_FSETCCS. Our target does which is how I found out the assert failure but I haven't implemented STRICT_FSETCCS yet.

OK, I see. I guess the patch LGTM then with the two issues fixed.

thopre updated this revision to Diff 307323.Nov 24 2020, 5:52 AM

Fix promotion for STRICT_FSETCCS

uweigand accepted this revision.Nov 24 2020, 6:23 AM

LGTM, thanks!

This revision is now accepted and ready to land.Nov 24 2020, 6:23 AM
This revision was landed with ongoing or failed builds.Nov 24 2020, 8:53 AM
This revision was automatically updated to reflect the committed changes.