[PATCH] D93974: [ValueTracking] Safe assumption context for args

Tue Jan 12 08:08:23 PST 2021

jdoerfert added a comment.

In D93974#2492566 <https://reviews.llvm.org/D93974#2492566>, @gilr wrote:

> In D93974#2489195 <https://reviews.llvm.org/D93974#2489195>, @jdoerfert wrote:
>
>> FWIW, I agree with @nikic, we should not put this logic here. There are two problems:
>>
>> 1. We compute something we might not need.
>> 2. We do it only in value tracking.
>
> Thanks for taking a look, Johannes!
>
>> I'd prefer the following, though the nullptr proposal seems fine too:
>> a) For arguments use the first instruction in the entry as context, this is trivial and correct.
>
> Agreed. Will limit this patch for this low-hanging fruit. Handling null contexts in isValidAssumptionForContext() is indeed more general, but also seems to have greater potential for causing trivial simplification of ephemeral values. If it works out it would replace the safe context for arguments.
>
>> b) When the context is used, e.g., to look for assumption, allow to some exploration of surrounding instructions.
>
> Not sure I see how (beyond the existing extension to reachable instructions). Since isValidAssumeForContext() can't tell why its context was chosen it must assume the context might be protecting an ephemeral from simplification, right?

The idea is that you can always do what you do here in `isValidAssumeForContext` (and friends). I just checked and we seem to do so already. Could you explain to me why we need to go for the `Last` instruction in this patch at all? What would happen if you simply pick the first in the entry block, which is trivially correct. (Note: You can skip llvm.assume to make some weird problems go away).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93974