This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add mayRaiseFPException to appropriate instructions
ClosedPublic

Authored by john.brawn on Dec 8 2021, 8:07 AM.

Details

Summary

This is mostly handled by adding "let mayRaiseFPException = 1" before the definition of the relevant instruction classes, but there are a couple of complications:

  • When we have a multiclass where currently some instantiations are of instructions that can raise an exception and others aren't we need to split that into two multiclasses, one inheriting from the other using a multiclass parameter to enable exceptions.
  • In a couple of places in the globalisel instruction selector we need to manually set the NoFPExcept flag. There's also another place that looks like it should need it, but that code is never hit for those opcodes due to them being handled by the generic instruction selector, so I've instead just removed them from the switch.

Diff Detail

Event Timeline

john.brawn created this revision.Dec 8 2021, 8:07 AM
john.brawn requested review of this revision.Dec 8 2021, 8:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2021, 8:07 AM

It looks like we have to do this very carefully to do it in a way that doesn't alter non-strict codegen for the worse, especially in making sure that nofpexcept flags are set everywhere. As far as I understand it should be an NFC there. The MachineCombiner isn't propagating flags at the moment, for example.

llvm/lib/Target/AArch64/AArch64InstrFormats.td
5037

Do we need to add this, or is it simpler to pass fpexceptions=0 from where it is used?

Matt added a subscriber: Matt.Jan 4 2022, 9:36 AM
john.brawn planned changes to this revision.Jan 31 2022, 5:23 AM

It looks like the machine combiner thing is a general problem with the AArch64 patterns not preserving MIFlags. I'll look into it.

john.brawn requested review of this revision.Jan 31 2022, 9:02 AM

I've created D118621 for making the AArch64 machine combiner patterns preserve MIFlags.

llvm/lib/Target/AArch64/AArch64InstrFormats.td
5037

Doing it this way makes things simpler for the next step, which is marking instructions as reading FPCR, as there's no direct way to express "this multiclass instantiation doesn't use a register that the multiclass does" and the best solution I've come up with is having it be decided in the multiclass by a multiclass parameter.

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 6:04 AM
john.brawn set the repository for this revision to rG LLVM Github Monorepo.

Rebase and ping.

The original version of this patch was altering the non-strict codegen because the flags were lost in the machine combiner. Have you done any other testing to make sure that doesn't happen anywhere else? To make sure this doesn't pessimise the performance for the default non-strict case.

The original version of this patch was altering the non-strict codegen because the flags were lost in the machine combiner. Have you done any other testing to make sure that doesn't happen anywhere else? To make sure this doesn't pessimise the performance for the default non-strict case.

I've build all of the MultiSource and SingleSource tests in llvm-test-suite with and without this patch (and those it depends on) at -O3 and there are no changes to the generated code.

dmgreen accepted this revision.Apr 4 2022, 3:54 AM

The original version of this patch was altering the non-strict codegen because the flags were lost in the machine combiner. Have you done any other testing to make sure that doesn't happen anywhere else? To make sure this doesn't pessimise the performance for the default non-strict case.

I've build all of the MultiSource and SingleSource tests in llvm-test-suite with and without this patch (and those it depends on) at -O3 and there are no changes to the generated code.

Thanks for checking. There might be other patterns that more rarely come up, but if they don't come up there and in the llvm tests then that's at least a good sign.

LGTM

This revision is now accepted and ready to land.Apr 4 2022, 3:54 AM
This revision was landed with ongoing or failed builds.Apr 14 2022, 8:52 AM
This revision was automatically updated to reflect the committed changes.