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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
