[PATCH] D45900: CodeGen: Fix invalid bitcast for lifetime.start/end

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In https://reviews.llvm.org/D45900#1093160, @rjmccall wrote:

> In https://reviews.llvm.org/D45900#1093154, @yaxunl wrote:
>
> > In https://reviews.llvm.org/D45900#1083377, @rjmccall wrote:
> >
> > > Oh, I see, it's not that the lifetime intrinsics don't handle pointers in the alloca address space, it's that we might have already promoted them into `DefaultAS`.
> > >
> > > Do the LLVM uses of lifetime intrinsics actually look through these address space casts?  I'm wondering if we might need to change how we emit the intrinsics so that they're emitted directly on (bitcasts of) the underlying allocas.
> >
> >
> > Some passes do not look through address space casts. Although there is InferAddressSpace pass which can eliminate the redundant address space casts, still it is desirable not to emit redundant address space in Clang.
> >
> > To avoid increasing complexity of alloca emitting API, I think we need a way to track the original alloca and the alloca casted to default address space. I can think of two ways:
> >
> > 1. add OriginalPointer member to Address, which is the originally emitted LLVM value for the variable. Whenever we pass the address of a variable we also pass the original LLVM value.
> > 2. add a map to CodeGenFunction to map the casted alloca to the real alloca.
> >
> >   Any suggestion? Thanks.
>
>
> Can we just call CreateLifetimeStart (and push the cleanup to call CreateLifetimeEnd) immediately after creating the alloca instead of waiting until later like we do now?
>
> Modifying Address is not appropriate, and adding a map to CGF would be big waste.


Since CreateTempAlloca returns the casted alloca by default, whereas CreateLifetimeStart expects the original alloca. If we want to call CreateLifetimeStart with original alloca, we have to do it in CreateTempAlloca.

This incurs two issues:

1. we need an enum parameter to control how lifetime.start/end is emitted. There are cases that lifetime.start is not emitted, and different ways to push cleanups for lifetime.end, e.g. in CodeGenFunction::

EmitMaterializeTemporaryExpr

  switch (M->getStorageDuration()) {
  case SD_Automatic:
  case SD_FullExpression:
    if (auto *Size = EmitLifetimeStart(
            CGM.getDataLayout().getTypeAllocSize(Object.getElementType()),
            Object.getPointer())) {
      if (M->getStorageDuration() == SD_Automatic)
        pushCleanupAfterFullExpr<CallLifetimeEnd>(NormalEHLifetimeMarker,
                                                  Object, Size);
      else
        pushFullExprCleanup<CallLifetimeEnd>(NormalEHLifetimeMarker, Object,
                                             Size);
    }
    break;

2. There are situations that the pushed cleanup for lifetime.end is deactivated and emitted early, e.g. in AggExprEmitter::withReturnValueSlot



  // If there's no dtor to run, the copy was the last use of our temporary.
  // Since we're not guaranteed to be in an ExprWithCleanups, clean up
  // eagerly.
  CGF.DeactivateCleanupBlock(LifetimeEndBlock, LifetimeStartInst);
  CGF.EmitLifetimeEnd(LifetimeSizePtr, RetAddr.getPointer());

In this case there is no good way to get the LifetimeStartInst and the original alloca inst.

Basically the emitting of lifetime.start/end is not uniform enough to be incorporated as part of CreateTempAlloca.

How about letting CreateTempAlloca have an optional pointer argument for returning the original alloca?


https://reviews.llvm.org/D45900