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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.



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*`?

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.


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

https://reviews.llvm.org/D127942