Page MenuHomePhabricator

[DAGCombiner] Fold BinOp into Select containing identity constant
Changes PlannedPublic

Authored by laytonio on Sat, Oct 24, 11:21 PM.

Details

Summary

To prevent materializing unneeded identity constants, and to avoid unnecessary arithmetic in branches that where the value wouldn't be modified, add folds for the following:

(add x, (select cc, 0, cf)) -> (select cc, x, (add x, cf))
(or x, (select cc, 0, cf)) -> (select cc, x, (or x, cf))
(xor x, (select cc, 0, cf)) -> (select cc, x, (xor x, cf))
(sub x, (select cc, 0, cf)) -> (select cc, x, (sub x, cf))
(shl x, (select cc, 0, cf)) -> (select cc, x, (shl x, cf))
(lshr x, (select cc, 0, cf)) -> (select cc, x, (lshr x, cf))
(ashr x, (select cc, 0, cf)) -> (select cc, x, (ashr x, cf))
(mul x, (select cc, 1, cf)) -> (select cc, x, (mul x, cf))
(sdiv x, (select cc, 1, cf)) -> (select cc, x, (sdiv x, cf))
(udiv x, (select cc, 1, cf)) -> (select cc, x, (udiv x, cf))
(and x, (select cc, -1, cf)) -> (select cc, x, (and x, cf))

Fixes select cases of PR47922

Diff Detail

Event Timeline

laytonio created this revision.Sat, Oct 24, 11:21 PM
laytonio requested review of this revision.Sat, Oct 24, 11:21 PM
laytonio added inline comments.Sat, Oct 24, 11:24 PM
llvm/test/CodeGen/PowerPC/cmpb.ll
243

Large regressions here unfortunately.

First attempt, feedback appreciated.

foad added a subscriber: foad.Mon, Oct 26, 1:51 AM
foad added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
536–537

Maybe pass IdentityInt in as an Optional<int64_t> to simplify all the callers?

2086–2087

"Don't do this if the select has more than one use", surely?

2118

Use int or unsigned instead of char.

llvm/test/CodeGen/AMDGPU/amdgpu-codegenprepare-idiv.ll
339–341

Regression here and in lots of other tests below. It looks like it's no longer using an add-with-carry instruction to implement x+(cond?1:0).

There's a large number of regressions in here that still need addressing - have you had any luck nailing any of them down?

llvm/test/CodeGen/AMDGPU/amdgpu-codegenprepare-idiv.ll
339–341

Is this just a generic combine we're missing or would this need to be fixed with an isel pattern?

llvm/test/CodeGen/X86/avx-select.ll
26

I'm not sure if these tests are testing what there were supposed to anymore?

llvm/test/CodeGen/X86/binop-identity.ll
3

Maybe pre-commit these tests to trunk and rebase this patch to show the delta?

I'm not sure you need -verify-machineinstrs

We're going to need a binop-identity-vector.ll set of tests as well.

laytonio updated this revision to Diff 300884.Mon, Oct 26, 9:13 PM

Fixed some of the regressions.

laytonio marked 2 inline comments as done.Mon, Oct 26, 9:16 PM
laytonio added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
536–537

I don't think there would be a good way to handle ISD::AND with AllOnes with a int64_t

laytonio added inline comments.Wed, Oct 28, 11:06 AM
llvm/test/CodeGen/X86/binop-identity.ll
3

We're going to need a binop-identity-vector.ll set of tests as well.

What vector widths do you think would be best for testing here? Should I do a left and right for each binop at say <4 x i32>, <8 x i32>, and <16 x i32> with SSE, AVX, and AVX512, or is that overkill?

laytonio planned changes to this revision.Wed, Oct 28, 11:23 AM
foad added inline comments.Thu, Oct 29, 6:57 AM
llvm/test/CodeGen/AMDGPU/dagcombine-select.ll
58–69

Regression here. The "or" is suppoed to be combined away.

llvm/test/CodeGen/AMDGPU/sdiv64.ll
576–578

Another add-with-carry regression here.

foad added inline comments.Thu, Oct 29, 7:00 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
536–537

I was thinking you'd pass in -1 for that case, which would be sign-extended whenever it gets converted to an APInt.