[PATCH] D105344: [DAGCombiner] Fold SETCC(FREEZE(x), y) to SETCC(x, y) if used by BRCOND

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added a comment.

In D105344#2872095 <https://reviews.llvm.org/D105344#2872095>, @nikic wrote:

> In D105344#2869740 <https://reviews.llvm.org/D105344#2869740>, @aqjune wrote:
>
>> This optimization seems still helpful after D105392 <https://reviews.llvm.org/D105392> is landed.
>> Should we generalize this transform and optimize away all freezes used by BRCOND? This will need an introduction of a new helper function that is analogous to `canCreateUndefOrPoison`.
>
> Hm, why do we need canCreateUndefOrPoison()?

It's because pushing freeze forward doesn't hold for certain poison-generating operations like `mul nsw`:
https://alive2.llvm.org/ce/z/boohtf
Actually, it depends on operations: `add nsw` is fine because `add nsw x, 0` is simply equivalent to `x` and `add nsw x, y` for any non-zero `y` there exists `x` that makes the result overflow.
Didn't think enough in which case the transformation is correct in general, yet.

> TBH I'm not quite sure when exactly doing this is valid. Let me think out load. Using a freeze instruction can only be necessary if either
>
> - It is used multiple times (or a recursive user is used multiple times). In this case freeze makes sure that the same value is seen by all users.
> - It is used in an instruction where undef/poison are immediate UB. In this case freeze launders UB.
>
> On the IR level this covers pretty much all uses, because we either have additional uses we don't see (function return/arg, store value) or the operand is UB on undef (branch, pointer operand). On the DAG level the issue with additional uses remains (store value, CopyToReg) but the UB on undef problem mostly goes away. At least we define BRCOND on undef as not UB -- I don't know about load/store addresses.
>
> So basically it's okay to drop freeze if we can walk a one-use chain down to a "sink" instruction that both doesn't have implicit uses and does not have UB on undef. One such sink is BRCOND. Whether instructions along the way can produce poison shouldn't matter.
>
> All that said, handling this fully generically is probably not simple, but writing it in a way that allows adding ISD opcodes to a whitelist as needed would probably still be nice.

I think splitting the transformation into smaller steps would be easier.
For some op1 and op2, folding `op2(op1(FREEZE(x),y))` => `op2(op1(x,y))` can be broken down into three steps:

    %z = op1(FREEZE(x), y)
    op2(%z)
  => // step 1
    %z = op1(FREEZE(x), FREEZE(y))
    op2(%z)
  => // step 2
    %z = op1(x, y)
    %z.fr = FREEZE(%z)
    op2(%z.fr)
  => // step 3
    %z = op1(x, y)
    op2(%z)

Step 1 is trivially correct.

Step 2: Relying on `canCreateUndefOrPoison` makes checking this step simpler, maybe?

Step 3: Optimizing freeze away is correct if the result of op2 on undef/poison is equivalent to op2 on some random number. In this case, op2 is BRCOND, and BRCOND(undef) is equivalent to BRCOND(0) or BRCOND(1), so this holds.
This property doesn't hold for e.g. `mul`; `mul undef, 2` returns a set of even numbers, whereas `mul n, 2` for any `n` is a single even number.

That being said, I'm fine with the current patch - it still allows optimizing the setcc-freeze.ll example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105344