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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93974