[PATCH] D115914: [InstCombine] Fold two PHI operands of `or` conditionally.

Wed Jan 12 17:44:19 PST 2022

luna added a comment.

In D115914#3237349 <https://reviews.llvm.org/D115914#3237349>, @spatel wrote:

> In D115914#3232967 <https://reviews.llvm.org/D115914#3232967>, @luna wrote:
>
>> thanks everyone for the feedback!
>>
>> carrot@ had a fix for another issue (in https://reviews.llvm.org/D116058) that has overlap with this one, but covering more cases like https://godbolt.org/z/or7W1b7eE and https://godbolt.org/z/MqTh5Te9c
>>
>> I'm going to hold this for a while; hopefully after https://reviews.llvm.org/D116058 is in, this patch is not needed.
>
> I don't think D116058 <https://reviews.llvm.org/D116058> is viable (and I'll comment there too). 
> This patch seems ok, but it's both too restrictive (only works with 'or') and potentially more costly than necessary (at least as a first step, we can check for constants instead of trying to simplify generally and fix the motivating tests). I put up an alternate patch: D117110 <https://reviews.llvm.org/D117110> .

thanks for the inputs! I agree that this patch is way too restrictive and not using the efficient matcher.

I went through D117110 <https://reviews.llvm.org/D117110> and learnt another way; only has one comment (for my learning) about whether it's idiomatic validate the newly inserted non-constant instruction could be simplified away in later iterations of instr-combine (basically, a stricter condition than only fold constant).

I'll probably revert this and https://reviews.llvm.org/D114832 after download a patch for my own back-up.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115914