[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via llvm-commits]

anna marked an inline comment as done.
anna added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1172
+      if (NumInstChecked++ > MaxInstCheckedForThrow ||
+          isGuaranteedToTransferExecutionToSuccessor(&I))
+        return true;
----------------
lebedev.ri wrote:
> anna wrote:
> > lebedev.ri wrote:
> > > anna wrote:
> > > > Noticed while adding couple more tests, there are 2 bugs here:
> > > > 1. the `isGuaranteedToTransferExecutionToSuccessor` check should be inverted
> > > > 2.  make_range should be until the return instruction - so we do not want `std::prev` on the returnInstruction. what's needed is: `make_range(RVal->getIterator(), RInst->getIterator())`
> > > > This means that from the callsite until (and excluding) the return instruction should be guaranteed to transfer execution to successor - only then we can backward propagate the attribute to that callsite.
> > > Are you aware of `llvm::isValidAssumeForContext()`?
> > > All this (including pitfalls) sound awfully close to that function.
> > as stated in a previous comment (https://reviews.llvm.org/D76140#1922292), adding `Assumes` here for simple cases seems like an overkill. It has significant IR churn and it also adds a use for something which can be easily inferred. 
> > Consider optimizations that depend on facts such as `hasOneUse` or a limited number of uses. We will now be inhibiting those optimizations. 
> While i venomously disagree with the avoidance of the usage of one versatile interface
> and hope things will change once there's more progress on attributor & assume bundles,
> in this case, as it can be seen even from the signature of the `isValidAssumeForContext()` function,
> it implies/forces nothing about using assumes, but only performs a validity checking,
> similar to the `MayContainThrowingOrExitingCall()`
> 
> https://github.com/llvm/llvm-project/blob/ca04d0c8fd269978be1c13fe1241172cdfe6a6ea/llvm/lib/Analysis/ValueTracking.cpp#L603
> 
> That being said, i haven't reviewed this code so maybe there's some differences here
> that make that function unapplicable here.
> 
`isValidAssumeForContext(Inv, CxtI, DT)` does not force anything about assumes, but AFAICT all code which uses this function either has some sort of guard in the caller that the instruction is an assume. Also, the comments in the code state that it is for an assume. In fact, I believe if we intend to use that function more widely for other purposes, we should rename the function before using it (just a thought), and currently we should assert that `Inv` is an assume. It captures the intent of the function.

That being said, I checked the code in `isValidAssumeForContext` and it does not fit the bill here for multiple reasons. We either do:
1. `isValidAssumeForContext(RVal /* Inv */, RInst /* CxtI  */)` which fails when we do not have DT and just return true when RVal comes before RInst - this is always the case, since RVal will come before RInst.
2. `isValidAssumeForContext(RInst /* Inv*/, RVal /* CxtI*/)` and it fails at the `!isEphemeralValueOf(Inv /* RI */, CxtI /* RV*/)` check.  
(By fail here, I mean, it does not have the same behaviour as `MayContainThrowingOrExitingCall`).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140