[PATCH] D127942: [NewGVN] add context instruction for SimplifyQuery

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D127942#3598316 <https://reviews.llvm.org/D127942#3598316>, @fhahn wrote:

> In D127942#3588327 <https://reviews.llvm.org/D127942#3588327>, @bcl5980 wrote:
>
>> In D127942#3588322 <https://reviews.llvm.org/D127942#3588322>, @nikic wrote:
>>
>>> SimplifyContext is created with `CxtI=nullptr` though, so I would expect it to not use a context instruction. Is the problem here that InstSimplify calls into ValueTracking helps like isKnownNonZero, and those convert CxtI==nullptr into the passed instruction in safeCxtI?
>>
>> Yes,  it call isKnownNonZero to fold `%tobool3.not.i = icmp eq i8 %lb1, 0` to false.
>
> I guess I am still not sure where and why a context instruction appears to be used here, as at the moment `SQ` never sets one and no instruction is passed to `simplify*`?

See https://github.com/llvm/llvm-project/blob/ae76b2f455016efb8cac5519d382be575b2d2edc/llvm/lib/Analysis/ValueTracking.cpp#L133. ValueTracking currently doesn't have a way to run completely without context instruction.

In D127942#3598316 <https://reviews.llvm.org/D127942#3598316>, @fhahn wrote:

> It's been a while since I had a close look at the code, but I *think* here we intentionally do not use any context information for instruction simplify (e.g. we also avoid using flags on instructions when looking through operands).Conceptually I think the expressions here are intended to be evaluated independently of their location. If we change that, I am worried that we will trade one bug for another, perhaps more subtle one. It *sounds* like it would be better to adjust the API to do not come up with a different context instruction, if no CxtI is added or `UseInstrInfo` is false.

I'm also somewhat apprehensive here -- the reason why I thought this would be safe is that this is part of the computation a specific instruction -- we don't fold every place where the same expression is used. However, the operands might come from a different context (which is why we also can't UseInstrInfo). In theory, I don't think that should be an issue, but in practice it may well be.


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

https://reviews.llvm.org/D127942