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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1172
+      if (NumInstChecked++ > MaxInstCheckedForThrow ||
+          isGuaranteedToTransferExecutionToSuccessor(&I))
+        return true;
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140