Skip to content

Commit 654756b

Browse files
ahatanakahatanak
committed
Address review comments
- Fix indentation. - Update comment for Address:ElementType. - Fix a bug where calls to getRawPointer in call argument expressions were causing the output to be dependent on the compiler.
1 parent d2d357b commit 654756b

File tree

8 files changed

+80
-79
lines changed

8 files changed

+80
-79
lines changed

‎clang/lib/CodeGen/Address.h

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -114,15 +114,9 @@ class Address {
114114
// The boolean flag indicates whether the pointer is known to be non-null.
115115
llvm::PointerIntPair<llvm::Value *, 1, bool> Pointer;
116116

117-
/// The expected IR type of the pointer. When the address is a raw pointer,
118-
/// this is currently redundant with the pointer's type, but for signed
119-
/// pointers it is useful if the pointer has been offsetted or cast from the
120-
/// original type. In the long run, when LLVM adopts opaque pointer types,
121-
/// this should become the notional element type of the address.
122-
///
123-
/// Carrying accurate element type information in Address makes it more
124-
/// convenient to work with Address values and allows frontend assertions to
125-
/// catch simple mistakes even after LLVM adopts opaque pointer types.
117+
/// The expected IR type of the pointer. Carrying accurate element type
118+
/// information in Address makes it more convenient to work with Address
119+
/// values and allows frontend assertions to catch simple mistakes.
126120
llvm::Type *ElementType = nullptr;
127121

128122
CharUnits Alignment;

‎clang/lib/CodeGen/CGAtomic.cpp

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1662,10 +1662,10 @@ std::pair<RValue, llvm::Value *> AtomicInfo::EmitAtomicCompareExchange(
16621662
if (shouldUseLibcall()) {
16631663
// Produce a source address.
16641664
Address ExpectedAddr = materializeRValue(Expected);
1665-
Address DesiredAddr = materializeRValue(Desired);
1666-
auto *Res = EmitAtomicCompareExchangeLibcall(
1667-
ExpectedAddr.getRawPointer(CGF), DesiredAddr.getRawPointer(CGF),
1668-
Success, Failure);
1665+
llvm::Value *ExpectedPtr = ExpectedAddr.getRawPointer(CGF);
1666+
llvm::Value *DesiredPtr = materializeRValue(Desired).getRawPointer(CGF);
1667+
auto *Res = EmitAtomicCompareExchangeLibcall(ExpectedPtr, DesiredPtr,
1668+
Success, Failure);
16691669
return std::make_pair(
16701670
convertAtomicTempToRValue(ExpectedAddr, AggValueSlot::ignored(),
16711671
SourceLocation(), /*AsValue=*/false),
@@ -1760,9 +1760,10 @@ void AtomicInfo::EmitAtomicUpdateLibcall(
17601760
AggValueSlot::ignored(),
17611761
SourceLocation(), /*AsValue=*/false);
17621762
EmitAtomicUpdateValue(CGF, *this, OldRVal, UpdateOp, DesiredAddr);
1763-
auto *Res = EmitAtomicCompareExchangeLibcall(ExpectedAddr.getRawPointer(CGF),
1764-
DesiredAddr.getRawPointer(CGF),
1765-
AO, Failure);
1763+
llvm::Value *ExpectedPtr = ExpectedAddr.getRawPointer(CGF);
1764+
llvm::Value *DesiredPtr = DesiredAddr.getRawPointer(CGF);
1765+
auto *Res =
1766+
EmitAtomicCompareExchangeLibcall(ExpectedPtr, DesiredPtr, AO, Failure);
17661767
CGF.Builder.CreateCondBr(Res, ExitBB, ContBB);
17671768
CGF.EmitBlock(ExitBB, /*IsFinished=*/true);
17681769
}
@@ -1842,9 +1843,10 @@ void AtomicInfo::EmitAtomicUpdateLibcall(llvm::AtomicOrdering AO,
18421843
CGF.Builder.CreateStore(OldVal, DesiredAddr);
18431844
}
18441845
EmitAtomicUpdateValue(CGF, *this, UpdateRVal, DesiredAddr);
1845-
auto *Res = EmitAtomicCompareExchangeLibcall(ExpectedAddr.getRawPointer(CGF),
1846-
DesiredAddr.getRawPointer(CGF),
1847-
AO, Failure);
1846+
llvm::Value *ExpectedPtr = ExpectedAddr.getRawPointer(CGF);
1847+
llvm::Value *DesiredPtr = DesiredAddr.getRawPointer(CGF);
1848+
auto *Res =
1849+
EmitAtomicCompareExchangeLibcall(ExpectedPtr, DesiredPtr, AO, Failure);
18481850
CGF.Builder.CreateCondBr(Res, ExitBB, ContBB);
18491851
CGF.EmitBlock(ExitBB, /*IsFinished=*/true);
18501852
}

‎clang/lib/CodeGen/CGBuilder.h

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -363,33 +363,33 @@ class CGBuilderTy : public CGBuilderBaseTy {
363363
using CGBuilderBaseTy::CreateMemCpy;
364364
llvm::CallInst *CreateMemCpy(Address Dest, Address Src, llvm::Value *Size,
365365
bool IsVolatile = false) {
366-
return CreateMemCpy(getRawPointerFromAddress(Dest),
367-
Dest.getAlignment().getAsAlign(),
368-
getRawPointerFromAddress(Src),
366+
llvm::Value *DestPtr = getRawPointerFromAddress(Dest);
367+
llvm::Value *SrcPtr = getRawPointerFromAddress(Src);
368+
return CreateMemCpy(DestPtr, Dest.getAlignment().getAsAlign(), SrcPtr,
369369
Src.getAlignment().getAsAlign(), Size, IsVolatile);
370370
}
371371
llvm::CallInst *CreateMemCpy(Address Dest, Address Src, uint64_t Size,
372372
bool IsVolatile = false) {
373-
return CreateMemCpy(getRawPointerFromAddress(Dest),
374-
Dest.getAlignment().getAsAlign(),
375-
getRawPointerFromAddress(Src),
373+
llvm::Value *DestPtr = getRawPointerFromAddress(Dest);
374+
llvm::Value *SrcPtr = getRawPointerFromAddress(Src);
375+
return CreateMemCpy(DestPtr, Dest.getAlignment().getAsAlign(), SrcPtr,
376376
Src.getAlignment().getAsAlign(), Size, IsVolatile);
377377
}
378378

379379
using CGBuilderBaseTy::CreateMemCpyInline;
380380
llvm::CallInst *CreateMemCpyInline(Address Dest, Address Src, uint64_t Size) {
381-
return CreateMemCpyInline(getRawPointerFromAddress(Dest),
382-
Dest.getAlignment().getAsAlign(),
383-
getRawPointerFromAddress(Src),
381+
llvm::Value *DestPtr = getRawPointerFromAddress(Dest);
382+
llvm::Value *SrcPtr = getRawPointerFromAddress(Src);
383+
return CreateMemCpyInline(DestPtr, Dest.getAlignment().getAsAlign(), SrcPtr,
384384
Src.getAlignment().getAsAlign(), getInt64(Size));
385385
}
386386

387387
using CGBuilderBaseTy::CreateMemMove;
388388
llvm::CallInst *CreateMemMove(Address Dest, Address Src, llvm::Value *Size,
389389
bool IsVolatile = false) {
390-
return CreateMemMove(getRawPointerFromAddress(Dest),
391-
Dest.getAlignment().getAsAlign(),
392-
getRawPointerFromAddress(Src),
390+
llvm::Value *DestPtr = getRawPointerFromAddress(Dest);
391+
llvm::Value *SrcPtr = getRawPointerFromAddress(Src);
392+
return CreateMemMove(DestPtr, Dest.getAlignment().getAsAlign(), SrcPtr,
393393
Src.getAlignment().getAsAlign(), Size, IsVolatile);
394394
}
395395

‎clang/lib/CodeGen/CGCall.cpp

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1148,9 +1148,10 @@ void CodeGenFunction::ExpandTypeToArgs(
11481148
}
11491149

11501150
/// Create a temporary allocation for the purposes of coercion.
1151-
static Address CreateTempAllocaForCoercion(CodeGenFunction &CGF, llvm::Type *Ty,
1152-
CharUnits MinAlign,
1153-
const Twine &Name = "tmp") {
1151+
static RawAddress CreateTempAllocaForCoercion(CodeGenFunction &CGF,
1152+
llvm::Type *Ty,
1153+
CharUnits MinAlign,
1154+
const Twine &Name = "tmp") {
11541155
// Don't use an alignment that's worse than what LLVM would prefer.
11551156
auto PrefAlign = CGF.CGM.getDataLayout().getPrefTypeAlign(Ty);
11561157
CharUnits Align = std::max(MinAlign, CharUnits::fromQuantity(PrefAlign));
@@ -1320,11 +1321,11 @@ static llvm::Value *CreateCoercedLoad(Address Src, llvm::Type *Ty,
13201321
}
13211322

13221323
// Otherwise do coercion through memory. This is stupid, but simple.
1323-
Address Tmp =
1324+
RawAddress Tmp =
13241325
CreateTempAllocaForCoercion(CGF, Ty, Src.getAlignment(), Src.getName());
13251326
CGF.Builder.CreateMemCpy(
1326-
Tmp.getRawPointer(CGF), Tmp.getAlignment().getAsAlign(),
1327-
Src.getRawPointer(CGF), Src.getAlignment().getAsAlign(),
1327+
Tmp.getPointer(), Tmp.getAlignment().getAsAlign(), Src.getRawPointer(CGF),
1328+
Src.getAlignment().getAsAlign(),
13281329
llvm::ConstantInt::get(CGF.IntPtrTy, SrcSize.getKnownMinValue()));
13291330
return CGF.Builder.CreateLoad(Tmp);
13301331
}
@@ -1408,11 +1409,12 @@ static void CreateCoercedStore(llvm::Value *Src,
14081409
//
14091410
// FIXME: Assert that we aren't truncating non-padding bits when have access
14101411
// to that information.
1411-
Address Tmp = CreateTempAllocaForCoercion(CGF, SrcTy, Dst.getAlignment());
1412+
RawAddress Tmp =
1413+
CreateTempAllocaForCoercion(CGF, SrcTy, Dst.getAlignment());
14121414
CGF.Builder.CreateStore(Src, Tmp);
14131415
CGF.Builder.CreateMemCpy(
14141416
Dst.getRawPointer(CGF), Dst.getAlignment().getAsAlign(),
1415-
Tmp.getRawPointer(CGF), Tmp.getAlignment().getAsAlign(),
1417+
Tmp.getPointer(), Tmp.getAlignment().getAsAlign(),
14161418
llvm::ConstantInt::get(CGF.IntPtrTy, DstSize.getFixedValue()));
14171419
}
14181420
}
@@ -3022,7 +3024,7 @@ void CodeGenFunction::EmitFunctionProlog(const CGFunctionInfo &FI,
30223024
// may be aliased, copy it to ensure that the parameter variable is
30233025
// mutable and has a unique adress, as C requires.
30243026
if (ArgI.getIndirectRealign() || ArgI.isIndirectAliased()) {
3025-
Address AlignedTemp = CreateMemTemp(Ty, "coerce");
3027+
RawAddress AlignedTemp = CreateMemTemp(Ty, "coerce");
30263028

30273029
// Copy from the incoming argument pointer to the temporary with the
30283030
// appropriate alignment.
@@ -3031,8 +3033,7 @@ void CodeGenFunction::EmitFunctionProlog(const CGFunctionInfo &FI,
30313033
// copy.
30323034
CharUnits Size = getContext().getTypeSizeInChars(Ty);
30333035
Builder.CreateMemCpy(
3034-
AlignedTemp.getRawPointer(*this),
3035-
AlignedTemp.getAlignment().getAsAlign(),
3036+
AlignedTemp.getPointer(), AlignedTemp.getAlignment().getAsAlign(),
30363037
ParamAddr.getRawPointer(*this),
30373038
ParamAddr.getAlignment().getAsAlign(),
30383039
llvm::ConstantInt::get(IntPtrTy, Size.getQuantity()));

‎clang/lib/CodeGen/CGException.cpp

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -397,27 +397,27 @@ namespace {
397397
void CodeGenFunction::EmitAnyExprToExn(const Expr *e, Address addr) {
398398
// Make sure the exception object is cleaned up if there's an
399399
// exception during initialization.
400-
pushFullExprCleanup<FreeException>(EHCleanup, addr.getRawPointer(*this));
401-
EHScopeStack::stable_iterator cleanup = EHStack.stable_begin();
402-
403-
// __cxa_allocate_exception returns a void*; we need to cast this
404-
// to the appropriate type for the object.
405-
llvm::Type *ty = ConvertTypeForMem(e->getType());
406-
Address typedAddr = addr.withElementType(ty);
407-
408-
// FIXME: this isn't quite right! If there's a final unelided call
409-
// to a copy constructor, then according to [except.terminate]p1 we
410-
// must call std::terminate() if that constructor throws, because
411-
// technically that copy occurs after the exception expression is
412-
// evaluated but before the exception is caught. But the best way
413-
// to handle that is to teach EmitAggExpr to do the final copy
414-
// differently if it can't be elided.
415-
EmitAnyExprToMem(e, typedAddr, e->getType().getQualifiers(),
416-
/*IsInit*/ true);
417-
418-
// Deactivate the cleanup block.
419-
DeactivateCleanupBlock(
420-
cleanup, cast<llvm::Instruction>(typedAddr.getRawPointer(*this)));
400+
pushFullExprCleanup<FreeException>(EHCleanup, addr.getRawPointer(*this));
401+
EHScopeStack::stable_iterator cleanup = EHStack.stable_begin();
402+
403+
// __cxa_allocate_exception returns a void*; we need to cast this
404+
// to the appropriate type for the object.
405+
llvm::Type *ty = ConvertTypeForMem(e->getType());
406+
Address typedAddr = addr.withElementType(ty);
407+
408+
// FIXME: this isn't quite right! If there's a final unelided call
409+
// to a copy constructor, then according to [except.terminate]p1 we
410+
// must call std::terminate() if that constructor throws, because
411+
// technically that copy occurs after the exception expression is
412+
// evaluated but before the exception is caught. But the best way
413+
// to handle that is to teach EmitAggExpr to do the final copy
414+
// differently if it can't be elided.
415+
EmitAnyExprToMem(e, typedAddr, e->getType().getQualifiers(),
416+
/*IsInit*/ true);
417+
418+
// Deactivate the cleanup block.
419+
DeactivateCleanupBlock(
420+
cleanup, cast<llvm::Instruction>(typedAddr.getRawPointer(*this)));
421421
}
422422

423423
Address CodeGenFunction::getExceptionSlot() {

‎clang/lib/CodeGen/CGExprCXX.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1278,8 +1278,9 @@ void CodeGenFunction::EmitNewArrayInitializer(
12781278

12791279
// Enter a partial-destruction Cleanup if necessary.
12801280
if (!CleanupDominator && needsEHCleanup(DtorKind)) {
1281-
pushRegularPartialArrayCleanup(BeginPtr.getRawPointer(*this),
1282-
CurPtr.getRawPointer(*this), ElementType,
1281+
llvm::Value *BeginPtrRaw = BeginPtr.getRawPointer(*this);
1282+
llvm::Value *CurPtrRaw = CurPtr.getRawPointer(*this);
1283+
pushRegularPartialArrayCleanup(BeginPtrRaw, CurPtrRaw, ElementType,
12831284
ElementAlign, getDestroyer(DtorKind));
12841285
Cleanup = EHStack.stable_begin();
12851286
CleanupDominator = Builder.CreateUnreachable();

‎clang/lib/CodeGen/CGOpenMPRuntime.cpp

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4482,12 +4482,12 @@ void CGOpenMPRuntime::emitUpdateClause(CodeGenFunction &CGF, LValue DepobjLVal,
44824482
FlagsLVal);
44834483

44844484
// Shift the address forward by one element.
4485-
Address ElementNext =
4486-
CGF.Builder.CreateConstGEP(Begin, /*Index=*/1, "omp.elementNext");
4487-
ElementPHI->addIncoming(ElementNext.getRawPointer(CGF),
4488-
CGF.Builder.GetInsertBlock());
4489-
llvm::Value *IsEmpty = CGF.Builder.CreateICmpEQ(
4490-
ElementNext.getRawPointer(CGF), End, "omp.isempty");
4485+
llvm::Value *ElementNext =
4486+
CGF.Builder.CreateConstGEP(Begin, /*Index=*/1, "omp.elementNext")
4487+
.getRawPointer(CGF);
4488+
ElementPHI->addIncoming(ElementNext, CGF.Builder.GetInsertBlock());
4489+
llvm::Value *IsEmpty =
4490+
CGF.Builder.CreateICmpEQ(ElementNext, End, "omp.isempty");
44914491
CGF.Builder.CreateCondBr(IsEmpty, DoneBB, BodyBB);
44924492
// Done.
44934493
CGF.EmitBlock(DoneBB, /*IsFinished=*/true);
@@ -7311,9 +7311,10 @@ class MappableExprsHandler {
73117311
CGF.EmitOMPSharedLValue(MC.getAssociatedExpression())
73127312
.getAddress(CGF);
73137313
}
7314-
Size = CGF.Builder.CreatePtrDiff(CGF.Int8Ty,
7315-
ComponentLB.getRawPointer(CGF),
7316-
LB.getRawPointer(CGF));
7314+
llvm::Value *ComponentLBPtr = ComponentLB.getRawPointer(CGF);
7315+
llvm::Value *LBPtr = LB.getRawPointer(CGF);
7316+
Size = CGF.Builder.CreatePtrDiff(CGF.Int8Ty, ComponentLBPtr,
7317+
LBPtr);
73177318
break;
73187319
}
73197320
}
@@ -7336,9 +7337,10 @@ class MappableExprsHandler {
73367337
CombinedInfo.DevicePtrDecls.push_back(nullptr);
73377338
CombinedInfo.DevicePointers.push_back(DeviceInfoTy::None);
73387339
CombinedInfo.Pointers.push_back(LB.getRawPointer(CGF));
7340+
llvm::Value *LBPtr = LB.getRawPointer(CGF);
73397341
Size = CGF.Builder.CreatePtrDiff(
73407342
CGF.Int8Ty, CGF.Builder.CreateConstGEP(HB, 1).getRawPointer(CGF),
7341-
LB.getRawPointer(CGF));
7343+
LBPtr);
73427344
CombinedInfo.Sizes.push_back(
73437345
CGF.Builder.CreateIntCast(Size, CGF.Int64Ty, /*isSigned=*/true));
73447346
CombinedInfo.Types.push_back(Flags);

‎clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1511,10 +1511,11 @@ static void shuffleAndStore(CodeGenFunction &CGF, Address SrcAddr,
15111511
Ptr = Address(PhiSrc, Ptr.getElementType(), Ptr.getAlignment());
15121512
ElemPtr =
15131513
Address(PhiDest, ElemPtr.getElementType(), ElemPtr.getAlignment());
1514-
llvm::Value *PtrDiff =
1515-
Bld.CreatePtrDiff(CGF.Int8Ty, PtrEnd.getRawPointer(CGF),
1516-
Bld.CreatePointerBitCastOrAddrSpaceCast(
1517-
Ptr.getRawPointer(CGF), CGF.VoidPtrTy));
1514+
llvm::Value *PtrEndRaw = PtrEnd.getRawPointer(CGF);
1515+
llvm::Value *PtrRaw = Ptr.getRawPointer(CGF);
1516+
llvm::Value *PtrDiff = Bld.CreatePtrDiff(
1517+
CGF.Int8Ty, PtrEndRaw,
1518+
Bld.CreatePointerBitCastOrAddrSpaceCast(PtrRaw, CGF.VoidPtrTy));
15181519
Bld.CreateCondBr(Bld.CreateICmpSGT(PtrDiff, Bld.getInt64(IntSize - 1)),
15191520
ThenBB, ExitBB);
15201521
CGF.EmitBlock(ThenBB);

0 commit comments

Comments
 (0)