This is an archive of the discontinued LLVM Phabricator instance.

[SPARC] Don't emit deprecated FP branches when targeting v9
ClosedPublic

Authored by koakuma on Oct 8 2022, 8:45 AM.

Details

Summary

Don't emit deprecated v8-style FP compares & branches when targeting v9 processors.
For now, always use %fcc0, because currently the allocator requires allocatable registers to also be spillable, which isn't the case with v9 FCC registers.
The work to enable allocation over the entire FCC register file will be done in a future patch.

Fixes bug #17834 (https://github.com/llvm/llvm-project/issues/17834).

Diff Detail

Event Timeline

koakuma created this revision.Oct 8 2022, 8:45 AM
koakuma requested review of this revision.Oct 8 2022, 8:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2022, 8:45 AM

I have a little bit of question regarding aesthetics: In the text assembly language, if the destination of FP compare is not specified, then it is taken as %fcc0.
(that is, fcmp<s|d|q> rs1, rs2 is encoded in the same way as fcmp<s|d|q> %fcc0, rs1, rs2 in the binary machine language)
Currently LLVM prefers to emit the former form. Is it okay to leave it as-is, or should it be changed so it explicitly spells out the %fcc0 destination?

arsenm added a subscriber: arsenm.Oct 8 2022, 9:13 AM
arsenm added inline comments.
llvm/lib/Target/Sparc/SparcISelLowering.cpp
3252

No else after return

llvm/lib/Target/Sparc/SparcInstrInfo.cpp
185 ↗(On Diff #466289)

I'm assuming it would still be valid to use the old opcodes on v9. You should explicitly handle the opcodes with the additional conditional register input, rather than relying on the subtarget check

arsenm added inline comments.Oct 8 2022, 9:18 AM
llvm/lib/Target/Sparc/SparcISelLowering.cpp
1916

Should add an optimization test for this. There are handful of DAG combines and I think codegenprepare also makes use of this

koakuma added inline comments.Oct 8 2022, 8:43 PM
llvm/lib/Target/Sparc/SparcISelLowering.cpp
1916

Would rerunning the tests in 2011-01-11-CC.ll with optimization enabled be enough for this, or do I need to add other tests too?

koakuma updated this revision to Diff 467767.Oct 14 2022, 6:48 AM

Codestyle changes + check branch opcode instead of subtarget when loading parameters.

koakuma updated this revision to Diff 467982.Oct 14 2022, 5:43 PM

Don't include unused headers.

arsenm added inline comments.Oct 14 2022, 5:45 PM
llvm/lib/Target/Sparc/SparcISelLowering.cpp
1916

I don't know, I would kind of doubt that this changes the output there

A question about the CI: It says that the build fails because of clang-format errors, yet when I run it locally it says "clang-format did not modify any files". What do I need to do?

llvm/lib/Target/Sparc/SparcISelLowering.cpp
1916

Hmm, I still don't know what extra optimizations are done by enabling setHasMultipleConditionRegisters so it's hard to me to make a suitable test case either...

Or, what about this: since currently I'm still unable to get LLVM to emit a code that would use more than one condition register, would it be okay if I remove that part for now and figure out how to deal with setHasMultipleConditionRegisters in a later patch?
For now, preventing it from emitting deprecated opcodes is enough for me.

koakuma updated this revision to Diff 469867.Oct 22 2022, 1:16 AM
koakuma edited the summary of this revision. (Show Details)

Don't do setHasMultipleConditionRegister.
For now, just disallow deprecated branch forms, the work of enabling whole-FP CCR allocation will be done in a separate patch.

koakuma updated this revision to Diff 473396.Nov 4 2022, 9:07 PM
koakuma edited the summary of this revision. (Show Details)

Simplify the implementation; in particular, do not trip into the allocator and force instructions to always use %fcc0.
This is to get around the limitation of needing allocatable registers to also be spillable, since, unfortunately, SPARC FCCs only support a very limited set of operations.

arsenm accepted this revision.Nov 16 2022, 4:22 PM

Test coverage looks thin

This revision is now accepted and ready to land.Nov 16 2022, 4:22 PM
This revision was automatically updated to reflect the committed changes.