This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][TargetLowering] Special case overflow expansion for (uaddo X, C).
ClosedPublic

Authored by HsiangKai on Apr 25 2022, 3:43 AM.

Diff Detail

Event Timeline

HsiangKai created this revision.Apr 25 2022, 3:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2022, 3:43 AM
HsiangKai requested review of this revision.Apr 25 2022, 3:43 AM
craig.topper added inline comments.Apr 25 2022, 8:57 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
8823–8826

Do we have any tests where RHS is a constant, but not a simm12 and needs to be placed in a register.

Would also be good to test cases where the setcc result is used by a branch. Even a simm12 would need to be put in a register for that case.

HsiangKai updated this revision to Diff 425107.Apr 25 2022, 7:37 PM

Add test cases.

HsiangKai updated this revision to Diff 425108.Apr 25 2022, 7:43 PM

Move new test cases to the end of the test file to remove unnecessary noises.

Update failed PowerPC test cases.

reames added a subscriber: reames.Apr 26 2022, 7:54 AM

It's not clear to my why preferring a comparison involving RHS over one involving LHS is generally profitable just because RHS happens to be a constant. Correct me if I'm wrong, but nothing has ensured that RHS is a small constant has it? Meaning, we may have to materialize it into a register and pay the same cost as the LHS based compare? By using RHS, we loose the ability to do CSE of repeated uadds.

I think you want some predicate to check that the comparison can fold into an immediate form at the minimum.

I'm new to this area, so I may simply be missing something here.

craig.topper added inline comments.Apr 26 2022, 8:15 AM
llvm/test/CodeGen/RISCV/xaluo.ll
4460

This is materializing a constant in a register which we didn't need to do before. Looking at the generated code before this patch, there was a move instruction, but I think that was because of register allocation constraints from above and below.

If we change the test to this, then this patch increases instructions.

define i64 @uaddo.i64.constant_setcc_on_overflow_flag(i64* %p) {
entry:
  %v1 = load i64, i64* %p
  %t = call {i64, i1} @llvm.uadd.with.overflow.i64(i64 %v1, i64 2)
  %val = extractvalue {i64, i1} %t, 0
  %obit = extractvalue {i64, i1} %t, 1
  br i1 %obit, label %IfOverflow, label %IfNoOverflow
IfOverflow:
  ret i64 0
IfNoOverflow:
  ret i64 %val
}
HsiangKai updated this revision to Diff 425734.Apr 28 2022, 2:57 AM

Add predicate for (uaddo X, C).

HsiangKai added inline comments.Apr 28 2022, 3:03 AM
llvm/test/CodeGen/RISCV/xaluo.ll
4460

Indeed, this patch increases instructions in this case. I have to think about it.

HsiangKai updated this revision to Diff 425921.Apr 28 2022, 4:21 PM

Revert the change and modify the comments only.

craig.topper added inline comments.Apr 28 2022, 5:26 PM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
8822–8823

"necessary benefits" -> "necessarily beneficial"

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6971–6972

Here too

6973

"especially the setcc" -> "especially when the setcc"

HsiangKai updated this revision to Diff 425941.Apr 28 2022, 6:32 PM

Address comments.

This revision is now accepted and ready to land.Apr 28 2022, 8:56 PM
This revision was landed with ongoing or failed builds.May 2 2022, 8:53 PM
This revision was automatically updated to reflect the committed changes.

Kind of late now, but I didn't notice the title wasn't updated. So it doesn't match the contents of the patch.