Page MenuHomePhabricator

[AArch64] Support SETCCCARRY lowering
ClosedPublic

Authored by fzhinkin on Oct 5 2022, 12:15 PM.

Details

Summary

Support SETCCCARRY lowering to SBCS instruction.

Related issue: https://github.com/llvm/llvm-project/issues/44629

Diff Detail

Event Timeline

fzhinkin created this revision.Oct 5 2022, 12:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 12:15 PM
fzhinkin updated this revision to Diff 465524.Oct 5 2022, 1:15 PM

Updated tests

fzhinkin added inline comments.Oct 5 2022, 1:17 PM
llvm/test/CodeGen/AArch64/i128-cmp.ll
24

Also checking if it's possible to use setcccarry to lower setcc eq/ne. It seems like it may improve code on x86, ARM and AArch64, but I'd prefer fixing it separately.

fzhinkin published this revision for review.Oct 6 2022, 12:28 AM
fzhinkin added reviewers: efriedma, samtebbs, dmgreen.
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2022, 12:28 AM
efriedma added inline comments.Oct 6 2022, 11:51 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
417

Do we ever actually use SETCCCARRY for i32 on targets where i64 is legal?

llvm/test/CodeGen/AArch64/i128-cmp.ll
24

Not sure how you could use setcccarry for equality; the carry bit doesn't contain enough information. You need something more like the AArch64 ccmp.

fzhinkin added inline comments.Oct 7 2022, 9:39 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
417

SelectionDAGLegalize::LegalizeOp uses default case to handle SETCCCARRY and calls TLI.getOperationAction with SETCCCARRY's type (which is i32). I guess that this question never arose previously because for other targets there were either no 64-bit registers (ARM), or because various value types were supported (X86, M86k).

Not sure if it should be fixed along with this change or via separate commit.

llvm/test/CodeGen/AArch64/i128-cmp.ll
24

Correct me if I'm wrong, but SETCCCARRY is like a regular SETCC which takes carry flag into account when comparing values. So we can generate the carry flag by comparing/subtracting registers holding least significant bits of i128 and then use SETCCCARRY to compare registers holding most significant bits (exactly how it works for other CCs).

It would be something like:

cmp x2, x0
sbcs xzr, x3, x1
cset w0, eq

Of course, on AArch64 sbcs in this case could be replaced with ccmp, but by using SETCCCARRY we can also improve code generated for other targets (X86 in particular). For targets supporting conditional comparisons (ARM, AArch64) SETCCARRY could then be replaced with more appropriate instruction.

efriedma accepted this revision.Oct 7 2022, 10:14 AM

LGTM

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
417

Oh, I see what you mean. The current way is fine; just wasn't really thinking about the output type.

llvm/test/CodeGen/AArch64/i128-cmp.ll
24
cmp x2, x0
sbcs xzr, x3, x1
cset w0, eq

Equality would generally mean x2 equals x0 and x3 equals x1. This throws away the equality bit of the first comparison, and adding the carry bit to the second comparison just means it returns "equal" in some cases where x3 and x1 aren't equal.

If you try to run some actual examples, I'm sure you'll see what I mean.

This revision is now accepted and ready to land.Oct 7 2022, 10:14 AM
fzhinkin marked 2 inline comments as done.Oct 7 2022, 10:44 AM
fzhinkin added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
417

I'm not sure that it supposed to work this way, because while SelectionDAGLegalize::LegalizeOp looks at SETCCCARRY's output type, DAGTypeLegalizer::IntegerExpandSetCCOperands uses operand's type.

llvm/test/CodeGen/AArch64/i128-cmp.ll
24

Got it, thanks!

efriedma added inline comments.Oct 7 2022, 11:02 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
417

Oh, hmm. They should use the same type; that's probably worth fixing. (I don't think it causes a practical problem here, but it could in other cases.)

fzhinkin marked an inline comment as done.Oct 7 2022, 11:13 AM
fzhinkin added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
417

Sure, I'll fix it first then, thanks!

fzhinkin updated this revision to Diff 466272.Oct 8 2022, 3:09 AM

Use operand's type to query SETCCCARRY legalize action

fzhinkin added inline comments.Oct 8 2022, 3:10 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
417

Added fix to this change as there were no other way to test its correctness.

This revision was automatically updated to reflect the committed changes.