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

[Gil Rapaport via Phabricator via llvm-commits]

gilr added a comment.

In D93974#2493403 <https://reviews.llvm.org/D93974#2493403>, @jdoerfert wrote:

> In D93974#2493287 <https://reviews.llvm.org/D93974#2493287>, @gilr wrote:
>
>> In D93974#2492979 <https://reviews.llvm.org/D93974#2492979>, @jdoerfert wrote:
>>
>>> 
>>
>>
>>
>>> 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).
>>
>> The patch indeed defaults to the first instruction in the entry. Scanning to the end at `safeCxtI()` was an optimization for cases where the first instruction is an ephemeral value of an assume (which seems quite likely for assumes about arguments) that would get the assume discarded by the `isEphemeralValueOf(Inv, CxtI)` check. At `isValidAssumeForContext()` we can't distinguish between a context given only as a control-flow marker and a context that also guards against simplifying an ephemeral so we can't try to improve it there, but since any context `safeCxtI()` provides for a null `CxtI` is just a control-flow marker anyway we might as well choose one that's not an ephemeral of any assume in the entry block. Does that make sense?
>
> Yes. But your code does "more" than that. All you want is this pseudo-code, right?
>
>   auto &It = EntryBlock.begin();
>   while (isaAssumeIntrinsic(*It)) ++It;
>   return *It;

Yes, if `isaAssumeIntrinsic` stands for "is an assume or an ephemeral of an assume". I suggested something like that in a previous comment:

> An unbounded scan of the entry block might indeed be too much even if applied only to arguments. The scan can perhaps be limited to some small number of instructions (5 seems like the minimum to cover most patterns in computeKnownBits()) which gets reset if an assume is encountered.

(which could still of course skip over perfectly good instructions and choose an ephemeral as context if the assumes were not written right at function entry)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93974