This is an archive of the discontinued LLVM Phabricator instance.

CodeGen: Fix invalid bitcast for lifetime.start/end
ClosedPublic

Authored by yaxunl on Apr 20 2018, 1:06 PM.

Details

Summary

lifetime.start/end expects pointer argument in alloca address space.
However in C++ a temporary variable is in default address space.

This patch changes API CreateMemTemp and CreateTempAlloca to
get the original alloca instruction and pass it lifetime.start/end.

It only affects targets with non-zero alloca address space.

Diff Detail

Event Timeline

yaxunl created this revision.Apr 20 2018, 1:06 PM

These functions must predate the addition of CreateLifetimeStart and CreateLifetimeEnd methods on IRBuilder; we should just be calling those.

yaxunl updated this revision to Diff 144574.Apr 30 2018, 9:57 AM

Revised by John's comments.

These functions must predate the addition of CreateLifetimeStart and CreateLifetimeEnd methods on IRBuilder; we should just be calling those.

Sorry. I think I misunderstood your comments. I will revise again.

yaxunl updated this revision to Diff 144600.Apr 30 2018, 11:25 AM

Call Builder.CreateLifetimeStart/End.

I cannot replace CodeGenFunction::EmitLifetimeStart with Builder.CreateLifetimeStart since we still need to check ShouldEmitLifetimeMarkers and do the address space cast.

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.

yaxunl added a comment.May 9 2018, 9:58 AM

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.
  1. add a map to CodeGenFunction to map the casted alloca to the real alloca.

Any suggestion? Thanks.

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.
  1. 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.

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.
  1. 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;
  1. 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?

That would work. I think it would be reasonable for AutoVarEmission to store that pointer if it makes things easier.

yaxunl updated this revision to Diff 146987.May 15 2018, 8:02 PM
yaxunl edited the summary of this revision. (Show Details)

Add optional argument to CreateMemTemp and CreateTempAlloca to get the original alloca and use it for lifetime intrinsic.

rjmccall accepted this revision.May 16 2018, 8:24 PM

LGTM.

This revision is now accepted and ready to land.May 16 2018, 8:24 PM
This revision was automatically updated to reflect the committed changes.