[PATCH] D65148: [SimplifyCFG] Bump phi-node-folding-threshold from 2 to 3

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Thank you for taking a look!

In D65148#1627466 <https://reviews.llvm.org/D65148#1627466>, @dmgreen wrote:

> For that test case above, it was to producing:
>
>   %conv2 = sext i16 %1 to i32
>   %add = add nsw i32 %conv2, %conv
>   %2 = icmp sgt i32 %add, -32768
>   %spec.select.i = select i1 %2, i32 %add, i32 -32768
>   %3 = icmp slt i32 %spec.select.i, 32767
>   %call7 = select i1 %3, i32 %spec.select.i, i32 32767
>   %conv3 = trunc i32 %call7 to i16
>   
>
> And Now:
>
>   %conv2 = sext i16 %1 to i32
>   %add = add nsw i32 %conv2, %conv
>   %2 = icmp slt i32 %add, 32768
>   %3 = icmp sgt i32 %add, -32768
>   %4 = select i1 %3, i32 %add, i32 -32768
>   %spec.select.i = select i1 %2, i32 %4, i32 32767
>   %conv3 = trunc i32 %spec.select.i to i16
>   
>
> Any idea why the `icmp slt i32 %spec.select.i, 32767` is now `icmp slt i32 %add, 32768`? Is that OK?


That is a valid transformation: https://rise4fun.com/Alive/bxq

> It looks a lot better than before, but means we need an extra "add 1" to materialise the constants.

Yeah, i spoke a *bit* too soon, realized that almost immediately after posting (https://reviews.llvm.org/D65765#1627080) :)

I need to follow-up with one more patch:

>   %2 = icmp slt i32 %add, 32768
>   %3 = icmp sgt i32 %add, -32768
>   %4 = select i1 %3, i32 %add, i32 -32768
>   %spec.select.i = select i1 %2, i32 %4, i32 32767

should be

>   %n3 = icmp sgt i32 %add, -32768
>   %n4 = select i1 %n3, i32 %add, i32 -32768
>   %n5 = icmp slt i32 %n4, 32768
>   %spec.select.i = select i1 %n5, i32 %n4, i32 32767

https://rise4fun.com/Alive/zRx0
I.e. in the last `select`, the `icmp` should be comparing the result of the first `select`, not the original input.
That will magically fix remaining issues (will allow the constant to get fixed to avoid that `add 1`)

So i suppose my question here is - other than this, did you observe any other regressions?
If not, care to tentatively stamp? I will *not* land this patch until after that last needed patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65148/new/

https://reviews.llvm.org/D65148