This is an archive of the discontinued LLVM Phabricator instance.

[LegalizeTypes][RISCV] Add a special case to ExpandIntRes_UADDSUBO for (uaddo X, 1).
ClosedPublic

Authored by craig.topper on Feb 22 2023, 10:15 PM.

Details

Summary

On targets that lack ADDCARRY support we split a wide uaddo into
an ADD and a SETCC that both need to be split.

For (uaddo X, 1) we can observe that when the add overflows the result
will be 0. We can emit (seteq (or Lo, Hi), 0) to detect this.

This improves D142071.

There is an alternative here. We could use either ~(lo(X) & hi(X)) == 0 or
(lo(X) & hi(X)) == -1 before the addition. That would be closer to the
code before D142071.

Diff Detail

Event Timeline

craig.topper created this revision.Feb 22 2023, 10:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2023, 10:15 PM
craig.topper requested review of this revision.Feb 22 2023, 10:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2023, 10:15 PM

LGTM. For optbranch_64, D144614 and D143646 get the same instruction after isel, but D144614 has one less assembly instruction (ret) than D143646, so D144614 is better

liaolucy accepted this revision.Feb 23 2023, 5:02 AM
This revision is now accepted and ready to land.Feb 23 2023, 5:02 AM

Both this and the related patch for ADD seem like they could be covered by a single set of DAG combine rules. Specifically:
a + 1 < a ==> a + 1 == 0
a - 1 > a ==> a + 1 == 0

(With the comparisons in the notation above being setcc nodes.)

Would doing the more general form and removing the expansion specific bits be worthwhile?

Both this and the related patch for ADD seem like they could be covered by a single set of DAG combine rules. Specifically:
a + 1 < a ==> a + 1 == 0
a - 1 > a ==> a + 1 == 0

I think there's a mistake there. I think this is correct.
a - 1 > a ==> a==0

(With the comparisons in the notation above being setcc nodes.)

Would doing the more general form and removing the expansion specific bits be worthwhile?

I'm not sure it helps with this patch, the wide setcc gets split before DAG combine would see it. But it could replace the change I did to ExpandIntRes_ADDSUB and the code I put in TargetLowering::expandUADDSUBO a long time back.