[PATCH] D46380: [InstSimplify] Adding safety checks for 'shl', 'ashr' and 'lshr'

[Omer Paparo Bivas via Phabricator via llvm-commits]

opaparo added a comment.

In https://reviews.llvm.org/D46380#1103696, @spatel wrote:

> In https://reviews.llvm.org/D46380#1098045, @opaparo wrote:
>
> > In https://reviews.llvm.org/D46380#1096108, @spatel wrote:
> >
> > > 1. This is an instsimplify patch - the review title should be changed; there should be minimal tests for each transform under tests/Transforms/InstSimplify.
> >
> >
> > The title was changed. However, those changes do not affect InstSimplify directly, as they are not used by this pass. The only functional change is in InstCombine, which uses these parts of InstSimplify as a library.
>
>
> I don't understand this statement. The proposal affects SimplifyWithOpReplaced() which is only called from within instsimplify (simplifySelectWithICmpCond), so every test should be visible using only -instsimplify. The first test should be reduced to something like this:
>
>   define i64 @sel_false_val_is_a_masked_shl_of_true_val1(i32 %x) {
>     %x15 = and i32 %x, 15
>     %sh = shl nuw nsw i32 %x15, 2
>     %z = zext i32 %sh to i64
>     %cmp = icmp eq i32 %x15, 0
>     %r = select i1 %cmp, i64 0, i64 %z
>     ret i64 %r
>   }
>  
>


You're right. I've originally missed the direct effect on InstSimplify, and I will change the tests location and structure if this change is accepted, but please also note that lib/Transforms/InstCombine/InstCombineSelect.cpp calls SimplifySelectInst which calls simplifySelectWithICmpCond which in turn calls SimplifyWithOpReplaced. Hence, InstCombine also benefits from this functional change.

> But this example doesn't need nsw/nuw, so what is this test trying to demonstrate?
>  https://rise4fun.com/Alive/9zX

Exactly that. In the current status, InstCombine and InstSimplify will not perform this transformation despite it's correctness. Furthermore, if you remove the nsw/nuw flags from the 'shl' in this example, InstCombine will first identify that the 'shl' really has no signed and unsigned wraps and will add the flags before the 'select' had the chance to optimize, which will result in the same un-transformed code. This is exactly one of the missed transformation opportunities I'm trying to cease.

> Also, this is still using ValueTracking, so I'm not comfortable continuing this review until we have more data about the cost and benefits.

You're right that ValueTracking is still used, but running this change on a large set of benchmark did not yield any noticeable compile-time regressions. In addition, the two functions from ValueTracking that I'm using (ComputeNumSignBits and MaskedValueIsZero) are already in use in InstructionSimplify.cpp.


https://reviews.llvm.org/D46380