[PATCH] D127202: [InlineFunction] Handle early exit during getUnderlyingObjects due to MaxLookup

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/test/Transforms/Coroutines/coro-retcon-opaque-ptr.ll:38
-; CHECK-NEXT:    call void @print(i32 5), !noalias !0
-; CHECK-NEXT:    call void @print(i32 6), !noalias !3
 ; CHECK-NEXT:    ret i32 0
----------------
nikic wrote:
> shchenz wrote:
> > nikic wrote:
> > > shchenz wrote:
> > > > nikic wrote:
> > > > > Do you know why these changes happen? (Just wondering, it looks harmless.)
> > > > The `!noalias` metadata is added when do inlining:
> > > > 
> > > > ```
> > > > define i32 @main() local_unnamed_addr {
> > > >   %cont1 = call i8* @f.resume.0(i8* nonnull %.sub, i1 zeroext false)
> > > > }
> > > > ```
> > > > 
> > > > ```
> > > > define internal i8* @f.resume.0(i8* noalias nocapture nonnull align 4 dereferenceable(8) %0, i1 zeroext %1) {
> > > > 
> > > >   %n.val.reload.addr = bitcast i8* %0 to i32*
> > > >   %n.val.reload = load i32, i32* %n.val.reload.addr, align 4
> > > >   %inc = add i32 %n.val.reload, 1
> > > >   store i32 %inc, i32* %n.val.reload.addr, align 4
> > > > 
> > > >   tail call void @print(i32 %inc)
> > > > }
> > > > ```
> > > > 
> > > > Before the change, underlying object `%inc = add i32 %n.val.reload, 1` for `tail call void @print(i32 %inc)` parameter is `RequiresNoCaptureBefore` and `PointerMayBeCapturedBefore()` returns false, so there will be `noalias` metadata added for the function call.
> > > > 
> > > > But after the change, underlying object `%inc = add i32 %n.val.reload, 1` is treated as a `UsesUnknownObject`.
> > > > 
> > > > Can the `add` (one operand is a load an the other is a constantInt) be handled in the `isEscapeSource()`?
> > > An `add` should never be an underlying object, because it's not a pointer.
> > > 
> > > I believe it gets added due to this code: https://github.com/llvm/llvm-project/blob/9081d3d8097af5e89905faf0ed568ba7e848e8df/llvm/lib/Transforms/Utils/InlineFunction.cpp#L1046-L1052 I don't think the comment there is correct, and we should drop the `IsArgMemOnlyCall &&` check. If a pointer is passed as an integer, then the ptrtoint will already capture it. And https://github.com/llvm/llvm-project/blob/9081d3d8097af5e89905faf0ed568ba7e848e8df/llvm/lib/Transforms/Utils/InlineFunction.cpp#L1111-L1114 will always check for captures for non-argmemonly function calls.
> > Yes, should I posted another patch to fix this issue? I think this fix should not reply on this patch, right? We can just ignore the non-pointer arguments in the PtrArgs collection phase and `PointerMayBeCapturedBefore()` will always check captures for the parameter even it is not a pointer for a non-argmemonly function calls.
> Yeah, sounds good.
Posted D128529. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127202