[PATCH] D78133: [PredicateInfo] Optionally set OriginalOp to renamed value it refers to.

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D78133#2134287 <https://reviews.llvm.org/D78133#2134287>, @fhahn wrote:

> In D78133#2131838 <https://reviews.llvm.org/D78133#2131838>, @nikic wrote:
>
> > Do we need to have this behind an option? That is, are there any PredicateInfo consumers who would //not// want this behavior?
>
>
> Currently NewGVN relies on the current behavior. It uses it to easily look up the original instruction and uses it to merge the metadata of replacement instructions (which is not an issue for SCCP because we don't replace the predicates with other equivalent instructions which could have metadata). I guess we could try to keep track of the original instruction in NewGVN (or traverse the chain there), but I don't think it's worth blocking the patch on the change to NewGVN.


Why don't we include both variants? While NewGVN needs the current OriginalOp for the replacement instruction patching, it also needs what is being implemented here (lets call it CondOp) when originally handling the PredicateInfo. As this comment indicates, NewGVN currently has the same problem as SCCP: https://github.com/llvm/llvm-project/blob/2279380eab08219911910e1ecdcef3eacb0b7f0c/llvm/lib/Transforms/Scalar/NewGVN.cpp#L1565-L1572

As PredicateInfo structures aren't particularly memory critical, I'd suggest to include both `OriginalOp` (value prior to any renaming) and `CondOp` (value used inside the condition) and use them as appropriate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78133