[PATCH] D27259: Make processing @llvm.assume more efficient - operand bundles

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D27259#617956, @sanjoy wrote:

> Hi Hal,
>
> Thanks for doing this!  I have not yet looked at the code in detail, but I have some high level comments, questions and answers.
>
> One high level question:  did you look at keeping this "affected" set as part of the `AssumptionCache` itself?  That is, keep a map that maps `ValueHandles` to the corresponding assume instruction they affect?


I thought about this, but I wanted to first see if we could get rid of the assumption cache entirely. I don't really like requiring the side contract which needs to be updated by every place that might clone instructions. If we can't eliminate it, then I agree that keeping this information in the cache is cleaner.

> Despite being an implementation detail we should add some basic documentation about this operand bundle to the langref.

Sure.

> I don't have a good solution for using this in SCEV.  One not-great solution is to have a pre-pass in SCEV that does this:
> 
>   for (AssumeInst AI : all_assumes_in_func) {
>     for (AffectOp : AI) {
>       AffectedMap[getSCEV(AffectOp)].push_back(AI);
>     }
>   }
>    
> 
> and then use the `AffectedMap` instead of using the `"affected"` operand bundle directly.

Hrmm. I don't really want to scan if possible - in part because it seems like it would make SCEV much less lazy than it is now (and in part because I want to get rid of the assumption cache, and if I do that, then I don't have an efficient way to find the assumptions). What if I hooked getSCEV so that, if the value in question was affected by an assumptions, then we add that to the AffectedMap?

The code that I'm currently trying to replace, as such, is this:

  // Check conditions due to any @llvm.assume intrinsics.
  for (auto &AssumeVH : AC.assumptions()) {
    if (!AssumeVH)
      continue;
    auto *CI = cast<CallInst>(AssumeVH);
    if (!DT.dominates(CI, Latch->getTerminator()))
      continue;
  
    if (isImpliedCond(Pred, LHS, RHS, CI->getArgOperand(0), false))
      return true;
  }

And so the problem is that I need to be able to figure out, given LHS and RHS, if the assumption condition might make  isImpliedCond(Pred, LHS, RHS, ...) true. getSCEV(affected_value) might not be exactly equal to wither LHS or RHS for this to be the case. Perhaps, however, if AffectedMap was essentially the transitive closure of the getSCEV(affected_value) -- meaning that the map contains those SCEVs but also all other SCEVs that use those as operands -- it might work?

> You could also try to scan a SCEV expression to fetch `Value` s out of `SCEVUnknown` nodes and use the use lists of those values.  But they won't catch cases like the loop preheader containing `assume(%a + 1 < %b)` and then a `isKnownPredicate(%a + 1 < %b)` query since `getSCEV("%a + 1")` will look through the addition and create an add expr.

Yea, this was exactly my concern with trying to use the values from SCEVUnknown.

> As for your question about guards:  while I initially wanted to use `AssumptionCache` (and I did have an RFC on llvm-dev about this), I did not go ahead with that idea since it wasn't obvious to me that it would be faster (that is, it isn't obviously a good idea; and I'd have to measure both sides to make an informed judgement).  This is because we tend to have lots of guards in every function, and looking at each one *could* get expensive.
> 
> However, I think we will be able to use this "affected" infrastructure more readily.

Sounds good. Thanks!


https://reviews.llvm.org/D27259