This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add `foldOverflowCheck` DAG combine
ClosedPublic

Authored by Kmeakin on Apr 14 2022, 3:52 AM.

Details

Reviewers
paulwalker-arm

Diff Detail

Event Timeline

Kmeakin created this revision.Apr 14 2022, 3:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2022, 3:52 AM
Kmeakin requested review of this revision.Apr 14 2022, 3:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2022, 3:52 AM
paulwalker-arm added inline comments.Apr 15 2022, 3:30 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
15427–15430

What's the value provided here? because I only see a single use.

15435

LLVM coding standard prefers early exits, so this should be

if (Op.getOpcode() == AArch64ISD::CSEL)
  return None;
15441

Unnecessary {} for single line block. Also applies to the next if block.

15478

I guess Dave will say OpLHS here? Likewise in getCSETCondCode.

15486–15487

Perhaps a silly question but this suggests

ADC l r (CMP (CSET HS carry) 1)) == ADC l r (CMP (CSET HI carry) 1))

which at first glance doesn't look correct. Looking at the test changes I only see instances of HS and LO. Sure HS and HI both set the carry flag but for the HI case that's not enough for CSET to return 1. Am I missing something?

15493

This seems weird. You either know CsetOp is a CSET or you don't. If you do then you know exactly which operand represents the carry?

15499

Does just Op->getVTList() work here?

18673–18678

How safe is this? i.e. do all the transformations relevant to ADD and SUB also apply to these nodes. My cursory glance suggests not. Even for the cases where the opcode is checked it seems inefficient to call a bunch of try... functions we know ahead of time are useless. There's an argument performAddSubCombine should be sliced in two given there's not much overlap between the ADD and SUB cases.

But that's not your problem so I guess my question is why not just have your new opcodes call directly into foldOverflowCheck? Also the ADDS and SUBS cases look redundant?

paulwalker-arm added inline comments.Apr 15 2022, 3:33 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
15435

Or rather

if (Op.getOpcode() != AArch64ISD::CSEL)
  return None;

:)

Kmeakin marked an inline comment as done.Apr 15 2022, 8:44 AM
Kmeakin added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
15427–15430

It is used in later patches

Kmeakin marked an inline comment as done.Apr 15 2022, 10:02 AM
Kmeakin updated this revision to Diff 423590.Apr 19 2022, 4:11 AM

Incorporate feedback from reviewers

Kmeakin marked 6 inline comments as done.Apr 19 2022, 4:13 AM
Kmeakin marked an inline comment as done.Apr 19 2022, 4:16 AM
Kmeakin updated this revision to Diff 423596.Apr 19 2022, 4:52 AM

Remove erroneous transformation of (ADC l r (CMP CSET HI carry) 1)) and (SBC l r (CMP (CSET LS carry) 1))

Kmeakin marked an inline comment as done.Apr 19 2022, 4:53 AM
paulwalker-arm accepted this revision.Apr 21 2022, 4:59 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
15454–15457

Given these are related perhaps merge these two if blocks?

This revision is now accepted and ready to land.Apr 21 2022, 4:59 AM
Kmeakin updated this revision to Diff 424160.Apr 21 2022, 5:22 AM

Merge ifs with same body

Kmeakin marked an inline comment as done.Apr 21 2022, 5:22 AM

We are noticing a clang crash after this commit: https://github.com/llvm/llvm-project/issues/55510