This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Improve foldOpIntoPhi() to use isImpliedCondition
ClosedPublic

Authored by kitaisreal on Jul 30 2023, 8:14 AM.

Diff Detail

Unit TestsFailed

Event Timeline

kitaisreal created this revision.Jul 30 2023, 8:14 AM
kitaisreal requested review of this revision.Jul 30 2023, 8:14 AM
nikic added inline comments.Jul 30 2023, 9:09 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
1363

There is an isImpliedByCondition() overload that accepts unpacked icmp operands. We should use that instead of cloning the instruction.

Updated implementation.

kitaisreal marked an inline comment as done.Jul 30 2023, 11:44 AM
nikic added inline comments.Jul 30 2023, 12:51 PM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
1364

I don't think the RHSOp0, RHSOp1 variables really add value over Ops[0], Ops[1] here.

1365

I think your tests currently only cover one of the LHSIsTrue cases. You also need one with successors swapped.

1372

You can use ConstantInt::getBool() here.

1389

Please extract the code above here into a separate function (which does simplifyInstructionWithOperands plus the special icmp case).

goldstein.w.n added inline comments.Jul 30 2023, 2:47 PM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
1370

can rename variable from 'Constant'. Its a big confusing to have variable name ==type.

kitaisreal marked 5 inline comments as done.

Updated implementation.

Looks basically fine to me.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
1338

I think this should check that getSuccessor(0) != getSuccessor(1). This will get canonicalized away, but is not guaranteed due to worklist order.

1345

I'd personally move this block to the end, because simplifyInstructionWithOperands() is the normal case, and this is a special case.

1355

Omit braces

kitaisreal marked 3 inline comments as done.

Updated implementation.

nikic accepted this revision.Jul 31 2023, 11:48 AM

LGTM

This revision is now accepted and ready to land.Jul 31 2023, 11:48 AM