This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv][AArch64] Add lowering of STRICT_FSETCC and STRICT_FSETCCS
ClosedPublic

Authored by john.brawn on Jan 24 2020, 9:25 AM.

Details

Summary

These become STRICT_FCMP and STRICT_FCMPE, when then get selected to the corresponding FCMP and FCMPE instructions, though the handling from there on isn't fully correct as we don't model reads and writes to FPCR and FPSR.

Diff Detail

Event Timeline

john.brawn created this revision.Jan 24 2020, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2020, 9:25 AM

Fix a silly error that I somehow failed to notice.

If ISD::STRICT_FSETCCS has a chain, does AArch64ISD::FCMPE need a chain?

If ISD::STRICT_FSETCCS has a chain, does AArch64ISD::FCMPE need a chain?

We should be doing that (and for FCMP as well), but it's currently there as a FIXME in line 5259. I'd thought that it would be tricky to do (it certainly is in AArch32 in D73194, due to some strange interaction with fmstat causing psr liveness to go wrong) but actually experimenting a bit it doesn't seem that hard. I'll have a new patch shortly.

john.brawn edited the summary of this revision. (Show Details)

Added a chain to FCMP and FCMPE.

Probably need to make the FCMP and FCMPE ISD opcodes return true for isTargetStrictFPOpcode(). This done by making the opcodes larger than a specific constant. I’m on my phone so I don’t remember the exact name.

llvm/lib/Target/AArch64/AArch64InstrInfo.td
3562

Do these need mayRaiseFPException?

john.brawn marked an inline comment as done.Jan 28 2020, 10:19 AM

Probably need to make the FCMP and FCMPE ISD opcodes return true for isTargetStrictFPOpcode(). This done by making the opcodes larger than a specific constant. I’m on my phone so I don’t remember the exact name.

Hmm, I've made FCMP and FCMPE be used for both strict and non-strict SETCC but probably we only want isTargetStrictFPOpcode when it really is strict. I'll make a separate STRICT_FCMP and STRICT_FCMPE instead (and looking at the X86 and SystemZ targets that's how they've done it as well).

llvm/lib/Target/AArch64/AArch64InstrInfo.td
3562

I'd rather not touch anything relating to modelling exception behaviour at the MachineInstr level at the moment, as I'm currently just working on getting SelectionDAG to stop hitting asserts.

john.brawn edited the summary of this revision. (Show Details)

Use separate strict opcodes instead of trying to share opcodes with non-strict.

kpn added a subscriber: kpn.Jan 29 2020, 1:32 PM
dmgreen accepted this revision.Jan 29 2020, 4:10 PM

Sounds good. I like how the code is shared between strict and non-strict.

LGTM

This revision is now accepted and ready to land.Jan 29 2020, 4:10 PM
This revision was automatically updated to reflect the committed changes.