This is an archive of the discontinued LLVM Phabricator instance.

[SPARC] Lower BR_CC to BPr on 64-bit target whenever possible
ClosedPublic

Authored by koakuma on Jan 24 2023, 5:34 AM.

Details

Summary

On 64-bit target, when doing i64 BR_CC where one of the comparison operands is a constant zero, try to fold the compare and BPcc into a BPr instruction.

For all integers, EQ and NE comparison are available, additionally for signed integers, GT, GE, LT, and LE is also available.

Depends on D142458 & D144012.

Diff Detail

Event Timeline

koakuma created this revision.Jan 24 2023, 5:34 AM
koakuma requested review of this revision.Jan 24 2023, 5:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2023, 5:34 AM
koakuma added inline comments.Jan 24 2023, 5:36 AM
llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp
45 ↗(On Diff #491730)

Not really sure if this is the proper way to do it, but I've found that without the left shift, the integrated assembler would misplace the two upper bits of BPr branch displacements.
Comments?

arsenm added inline comments.Feb 2 2023, 6:29 PM
llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp
45 ↗(On Diff #491730)

I know nothing about this, but it seems like it should be split into a separate patch

koakuma added inline comments.Feb 2 2023, 7:30 PM
llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp
45 ↗(On Diff #491730)

I'm afraid I couldn't split this, since the misplacement of the upper two bits would result in all backwards BPr branches being miscompiled.

Ping again?

arsenm added inline comments.Feb 13 2023, 11:23 AM
llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp
45 ↗(On Diff #491730)

But this does something maybe wrong as-is. This should be separately testable with a pure MC test (e.g. llvm/test/MC/AMDGPU/offsetbug_once.s)

koakuma updated this revision to Diff 497318.Feb 14 2023, 7:15 AM
koakuma edited the summary of this revision. (Show Details)

Split the encoding fix into D144012.

arsenm accepted this revision.Feb 14 2023, 1:01 PM
This revision is now accepted and ready to land.Feb 14 2023, 1:01 PM
This revision was landed with ongoing or failed builds.Mar 11 2023, 3:53 PM
This revision was automatically updated to reflect the committed changes.

Sorry, just noticed that this was being merged, but can we postpone/revert it at least until D144012 is also merged, @brad?
Without it, backwards branches might be miscompiled when using IAS...

brad added a comment.Mar 11 2023, 11:11 PM

Sorry, just noticed that this was being merged, but can we postpone/revert it at least until D144012 is also merged, @brad?

Just D142461?

Just D142461?

Yes, just this one (D142461). Thanks a lot!

@brad, now that D144012 has been merged, this should be okay to merge too.

brad added a comment.May 5 2023, 10:31 AM

@brad, now that D144012 has been merged, this should be okay to merge too.

This was not reverted. It's already in the tree.