This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] simplify a cmp+select using binop result equivalence
ClosedPublic

Authored by spatel on Aug 1 2019, 7:13 AM.

Details

Summary

As discussed in PR42696:
https://bugs.llvm.org/show_bug.cgi?id=42696
...but won't help that case yet.

We have an odd situation where a select equivalence fold was implemented in InstSimplify when it could have been done more generally in InstCombine if we allow dropping of {nsw,nuw,exact} from a binop operand.

Here's an example:
https://rise4fun.com/Alive/Xplr

%cmp = icmp eq i32 %x, 2147483647
%add = add nsw i32 %x, 1
%sel = select i1 %cmp, i32 -2147483648, i32 %add
=>
%sel = add i32 %x, 1

I've left the InstSimplify code in place for now, but my guess is that we'd prefer to remove that as a follow-up to save on code duplication and compile-time.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Aug 1 2019, 7:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2019, 7:13 AM
spatel updated this revision to Diff 212834.Aug 1 2019, 9:11 AM

Patch updated:
There was a bug in translating the code from instsimplify. Added tests (rL367577, rL367579) to make sure we are propagating the correct T/F arm of the select with swapped operands and 'ne' predicate.

I'm having trouble grasping all that code duplication.
Can that be generalized+parametrized a bit?
Also, please can you add the example with explanation into that function?

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
1091 ↗(On Diff #212834)

Sure not <Instruction> ?

spatel marked 2 inline comments as done.Aug 2 2019, 6:47 AM

I'm having trouble grasping all that code duplication.
Can that be generalized+parametrized a bit?
Also, please can you add the example with explanation into that function?

Agree - I was afraid of introducing a bug while copy/pasting this from the InstSimplify blob, so amplified the existing mess...and managed to introduce a bug anyway.
I'll shrink it and add more comments. Interestingly, the existing tests show that we're missing some wrapping analysis by performing the transform in instsimplify (ie, the test names suggesting what is or isn't safe might be bogus).

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
1091 ↗(On Diff #212834)

Yes - it's limited to BinOps for the moment, but that may change.

spatel updated this revision to Diff 213044.Aug 2 2019, 7:07 AM
spatel marked an inline comment as done.

Patch updated:

  1. Simplified code.
  2. Added examples to code comments.
  3. Changed dyn_cast from BinaryOperator to more general Instruction.
lebedev.ri accepted this revision.Aug 2 2019, 8:03 AM

Thanks, now it makes sense!
Looks good to me.

This revision is now accepted and ready to land.Aug 2 2019, 8:03 AM
This revision was automatically updated to reflect the committed changes.