[Coroutines] Do not add allocas for retcon coroutines

[sebastian-ne]

Summary

Same as for async-style lowering, if there are no resume points in a
function, the coroutine frame pointer will be replaced by an undef,
making all accesses to the frame undefinde behavior.

Fix this by not adding allocas to the coroutine frame if there are no
resume points.

The test is new, I'll pre-commit if this is accepted.

[ChuanqiXu]

To be honest, I'm not familiar with retcon ABI. But the explanation, the patch itself looks good. If you want to find a better reviewer, you can use git log for the coroutines updates.

[jsilvanus]

Not your code, but:

Maybe it would be cleaner to check whether we need a frame object at all instead of adding special case handling for allocas?
Then, based on that condition, we could skip setup of the frame type and pointer, and leave the alloca's unchanged.

If that's too much of a change, I'm fine with the proposed change, as it just fixes the existing condition.

llvm/test/Transforms/Coroutines/coro-retcon.ll

142

Note the unused frame pointer here.

[sebastian-ne]

In D137866#3924203, @jsilvanus wrote:

Not your code, but:

Maybe it would be cleaner to check whether we need a frame object at all instead of adding special case handling for allocas?
Then, based on that condition, we could skip setup of the frame type and pointer, and leave the alloca's unchanged.

If that's too much of a change, I'm fine with the proposed change, as it just fixes the existing condition.

I committed this for now as it just extends the existing handling for async to retcon. I’ll take a look at implementing your suggestion.

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

for (int Repeat = 0; Repeat < 4; ++Repeat) {
       Spills.clear();
     }
   }
 
   if (Shape.ABI != coro::ABI::Async && Shape.ABI != coro::ABI::Retcon &&
       Shape.ABI != coro::ABI::RetconOnce)
     sinkLifetimeStartMarkers(F, Shape, Checker);
 
-  if (Shape.ABI != coro::ABI::Async || !Shape.CoroSuspends.empty())
+  if (Shape.ABI == coro::ABI::Switch || !Shape.CoroSuspends.empty())
     collectFrameAllocas(F, Shape, Checker, FrameData.Allocas);
   LLVM_DEBUG(dumpAllocas(FrameData.Allocas));
 
   // Collect the spills for arguments and other not-materializable values.
   for (Argument &A : F.args())
     for (User *U : A.users())
       if (Checker.isDefinitionAcrossSuspend(A, U))
         FrameData.Spills[&A].push_back(cast<Instruction>(U));

llvm/test/Transforms/Coroutines/coro-retcon.ll

 cleanup:
   call i1 @llvm.coro.end(i8* %hdl, i1 0)
   unreachable
 }
 
 define i8* @nosuspend(i8* %buffer, i32 %n) {
 ; CHECK-LABEL: @nosuspend(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    unreachable
+; CHECK-NEXT:    [[A:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    store i32 [[N:%.*]], i32* [[A]], align 4
+; CHECK-NEXT:    call void @use_var_ptr(i32* nonnull [[A]])
+; CHECK-NEXT:    [[AL:%.*]] = load i32, i32* [[A]], align 4
+; CHECK-NEXT:    call void @use_var(i32 [[AL]])
+; CHECK-NEXT:    ret i8* null
 ;
 ; CORO-LABEL: @nosuspend(
 ; CORO-NEXT:  entry:
 ; CORO-NEXT:    [[FRAMEPTR:%.*]] = bitcast i8* undef to %nosuspend.Frame*
-; CORO-NEXT:    [[A:%.*]] = getelementptr inbounds [[NOSUSPEND_FRAME:%.*]], %nosuspend.Frame* [[FRAMEPTR]], i32 0, i32 0
+; CORO-NEXT:    [[A:%.*]] = alloca i32, align 4
 ; CORO-NEXT:    store i32 [[N:%.*]], i32* [[A]], align 4
 ; CORO-NEXT:    call void @use_var_ptr(i32* [[A]])
 ; CORO-NEXT:    [[AL:%.*]] = load i32, i32* [[A]], align 4
 ; CORO-NEXT:    call void @use_var(i32 [[AL]])
 ; CORO-NEXT:    ret i8* null
 ;
 entry:
   %id = call token @llvm.coro.id.retcon(i32 8, i32 4, i8* %buffer, i8* bitcast (i8* (i8*, i1)* @prototype to i8*), i8* bitcast (i8* (i32)* @allocate to i8*), i8* bitcast (void (i8*)* @deallocate to i8*))