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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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()? 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105344