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.

Author: ahatanak

clang/lib/CodeGen/Address.h

@@ -114,15 +114,9 @@ class Address {
   // The boolean flag indicates whether the pointer is known to be non-null.
   llvm::PointerIntPair<llvm::Value *, 1, bool> Pointer;
 
-  /// The expected IR type of the pointer. When the address is a raw pointer,
-  /// this is currently redundant with the pointer's type, but for signed
-  /// pointers it is useful if the pointer has been offsetted or cast from the
-  /// original type. In the long run, when LLVM adopts opaque pointer types,
-  /// this should become the notional element type of the address.
-  ///
-  /// Carrying accurate element type information in Address makes it more
-  /// convenient to work with Address values and allows frontend assertions to
-  /// catch simple mistakes even after LLVM adopts opaque pointer types.
+  /// The expected IR type of the pointer. Carrying accurate element type
+  /// information in Address makes it more convenient to work with Address
+  /// values and allows frontend assertions to catch simple mistakes.
   llvm::Type *ElementType = nullptr;
 
   CharUnits Alignment;

clang/lib/CodeGen/CGAtomic.cpp

@@ -1901,10 +1901,10 @@ std::pair<RValue, llvm::Value *> AtomicInfo::EmitAtomicCompareExchange(
   if (shouldUseLibcall()) {
     // Produce a source address.
     Address ExpectedAddr = materializeRValue(Expected);
-    Address DesiredAddr = materializeRValue(Desired);
-    auto *Res = EmitAtomicCompareExchangeLibcall(
-        ExpectedAddr.getRawPointer(CGF), DesiredAddr.getRawPointer(CGF),
-        Success, Failure);
+    llvm::Value *ExpectedPtr = ExpectedAddr.getRawPointer(CGF);
+    llvm::Value *DesiredPtr = materializeRValue(Desired).getRawPointer(CGF);
+    auto *Res = EmitAtomicCompareExchangeLibcall(ExpectedPtr, DesiredPtr,
+                                                 Success, Failure);
     return std::make_pair(
         convertAtomicTempToRValue(ExpectedAddr, AggValueSlot::ignored(),
                                   SourceLocation(), /*AsValue=*/false),
@@ -1999,9 +1999,10 @@ void AtomicInfo::EmitAtomicUpdateLibcall(
                                            AggValueSlot::ignored(),
                                            SourceLocation(), /*AsValue=*/false);
   EmitAtomicUpdateValue(CGF, *this, OldRVal, UpdateOp, DesiredAddr);
-  auto *Res = EmitAtomicCompareExchangeLibcall(ExpectedAddr.getRawPointer(CGF),
-                                               DesiredAddr.getRawPointer(CGF),
-                                               AO, Failure);
+  llvm::Value *ExpectedPtr = ExpectedAddr.getRawPointer(CGF);
+  llvm::Value *DesiredPtr = DesiredAddr.getRawPointer(CGF);
+  auto *Res =
+      EmitAtomicCompareExchangeLibcall(ExpectedPtr, DesiredPtr, AO, Failure);
   CGF.Builder.CreateCondBr(Res, ExitBB, ContBB);
   CGF.EmitBlock(ExitBB, /*IsFinished=*/true);
 }
@@ -2081,9 +2082,10 @@ void AtomicInfo::EmitAtomicUpdateLibcall(llvm::AtomicOrdering AO,
     CGF.Builder.CreateStore(OldVal, DesiredAddr);
   }
   EmitAtomicUpdateValue(CGF, *this, UpdateRVal, DesiredAddr);
-  auto *Res = EmitAtomicCompareExchangeLibcall(ExpectedAddr.getRawPointer(CGF),
-                                               DesiredAddr.getRawPointer(CGF),
-                                               AO, Failure);
+  llvm::Value *ExpectedPtr = ExpectedAddr.getRawPointer(CGF);
+  llvm::Value *DesiredPtr = DesiredAddr.getRawPointer(CGF);
+  auto *Res =
+      EmitAtomicCompareExchangeLibcall(ExpectedPtr, DesiredPtr, AO, Failure);
   CGF.Builder.CreateCondBr(Res, ExitBB, ContBB);
   CGF.EmitBlock(ExitBB, /*IsFinished=*/true);
 }

clang/lib/CodeGen/CGBuilder.h

@@ -363,33 +363,33 @@ class CGBuilderTy : public CGBuilderBaseTy {
   using CGBuilderBaseTy::CreateMemCpy;
   llvm::CallInst *CreateMemCpy(Address Dest, Address Src, llvm::Value *Size,
                                bool IsVolatile = false) {
-    return CreateMemCpy(getRawPointerFromAddress(Dest),
-                        Dest.getAlignment().getAsAlign(),
-                        getRawPointerFromAddress(Src),
+    llvm::Value *DestPtr = getRawPointerFromAddress(Dest);
+    llvm::Value *SrcPtr = getRawPointerFromAddress(Src);
+    return CreateMemCpy(DestPtr, Dest.getAlignment().getAsAlign(), SrcPtr,
                         Src.getAlignment().getAsAlign(), Size, IsVolatile);
   }
   llvm::CallInst *CreateMemCpy(Address Dest, Address Src, uint64_t Size,
                                bool IsVolatile = false) {
-    return CreateMemCpy(getRawPointerFromAddress(Dest),
-                        Dest.getAlignment().getAsAlign(),
-                        getRawPointerFromAddress(Src),
+    llvm::Value *DestPtr = getRawPointerFromAddress(Dest);
+    llvm::Value *SrcPtr = getRawPointerFromAddress(Src);
+    return CreateMemCpy(DestPtr, Dest.getAlignment().getAsAlign(), SrcPtr,
                         Src.getAlignment().getAsAlign(), Size, IsVolatile);
   }
 
   using CGBuilderBaseTy::CreateMemCpyInline;
   llvm::CallInst *CreateMemCpyInline(Address Dest, Address Src, uint64_t Size) {
-    return CreateMemCpyInline(getRawPointerFromAddress(Dest),
-                              Dest.getAlignment().getAsAlign(),
-                              getRawPointerFromAddress(Src),
+    llvm::Value *DestPtr = getRawPointerFromAddress(Dest);
+    llvm::Value *SrcPtr = getRawPointerFromAddress(Src);
+    return CreateMemCpyInline(DestPtr, Dest.getAlignment().getAsAlign(), SrcPtr,
                               Src.getAlignment().getAsAlign(), getInt64(Size));
   }
 
   using CGBuilderBaseTy::CreateMemMove;
   llvm::CallInst *CreateMemMove(Address Dest, Address Src, llvm::Value *Size,
                                 bool IsVolatile = false) {
-    return CreateMemMove(getRawPointerFromAddress(Dest),
-                         Dest.getAlignment().getAsAlign(),
-                         getRawPointerFromAddress(Src),
+    llvm::Value *DestPtr = getRawPointerFromAddress(Dest);
+    llvm::Value *SrcPtr = getRawPointerFromAddress(Src);
+    return CreateMemMove(DestPtr, Dest.getAlignment().getAsAlign(), SrcPtr,
                          Src.getAlignment().getAsAlign(), Size, IsVolatile);
   }
 

clang/lib/CodeGen/CGCall.cpp

@@ -1144,9 +1144,10 @@ void CodeGenFunction::ExpandTypeToArgs(
 }
 
 /// Create a temporary allocation for the purposes of coercion.
-static Address CreateTempAllocaForCoercion(CodeGenFunction &CGF, llvm::Type *Ty,
-                                           CharUnits MinAlign,
-                                           const Twine &Name = "tmp") {
+static RawAddress CreateTempAllocaForCoercion(CodeGenFunction &CGF,
+                                              llvm::Type *Ty,
+                                              CharUnits MinAlign,
+                                              const Twine &Name = "tmp") {
   // Don't use an alignment that's worse than what LLVM would prefer.
   auto PrefAlign = CGF.CGM.getDataLayout().getPrefTypeAlign(Ty);
   CharUnits Align = std::max(MinAlign, CharUnits::fromQuantity(PrefAlign));
@@ -1318,11 +1319,11 @@ static llvm::Value *CreateCoercedLoad(Address Src, llvm::Type *Ty,
   }
 
   // Otherwise do coercion through memory. This is stupid, but simple.
-  Address Tmp =
+  RawAddress Tmp =
       CreateTempAllocaForCoercion(CGF, Ty, Src.getAlignment(), Src.getName());
   CGF.Builder.CreateMemCpy(
-      Tmp.getRawPointer(CGF), Tmp.getAlignment().getAsAlign(),
-      Src.getRawPointer(CGF), Src.getAlignment().getAsAlign(),
+      Tmp.getPointer(), Tmp.getAlignment().getAsAlign(), Src.getRawPointer(CGF),
+      Src.getAlignment().getAsAlign(),
       llvm::ConstantInt::get(CGF.IntPtrTy, SrcSize.getKnownMinValue()));
   return CGF.Builder.CreateLoad(Tmp);
 }
@@ -1406,11 +1407,12 @@ static void CreateCoercedStore(llvm::Value *Src,
     //
     // FIXME: Assert that we aren't truncating non-padding bits when have access
     // to that information.
-    Address Tmp = CreateTempAllocaForCoercion(CGF, SrcTy, Dst.getAlignment());
+    RawAddress Tmp =
+        CreateTempAllocaForCoercion(CGF, SrcTy, Dst.getAlignment());
     CGF.Builder.CreateStore(Src, Tmp);
     CGF.Builder.CreateMemCpy(
         Dst.getRawPointer(CGF), Dst.getAlignment().getAsAlign(),
-        Tmp.getRawPointer(CGF), Tmp.getAlignment().getAsAlign(),
+        Tmp.getPointer(), Tmp.getAlignment().getAsAlign(),
         llvm::ConstantInt::get(CGF.IntPtrTy, DstSize.getFixedValue()));
   }
 }
@@ -3004,7 +3006,7 @@ void CodeGenFunction::EmitFunctionProlog(const CGFunctionInfo &FI,
         // may be aliased, copy it to ensure that the parameter variable is
         // mutable and has a unique adress, as C requires.
         if (ArgI.getIndirectRealign() || ArgI.isIndirectAliased()) {
-          Address AlignedTemp = CreateMemTemp(Ty, "coerce");
+          RawAddress AlignedTemp = CreateMemTemp(Ty, "coerce");
 
           // Copy from the incoming argument pointer to the temporary with the
           // appropriate alignment.
@@ -3013,8 +3015,7 @@ void CodeGenFunction::EmitFunctionProlog(const CGFunctionInfo &FI,
           // copy.
           CharUnits Size = getContext().getTypeSizeInChars(Ty);
           Builder.CreateMemCpy(
-              AlignedTemp.getRawPointer(*this),
-              AlignedTemp.getAlignment().getAsAlign(),
+              AlignedTemp.getPointer(), AlignedTemp.getAlignment().getAsAlign(),
               ParamAddr.getRawPointer(*this),
               ParamAddr.getAlignment().getAsAlign(),
               llvm::ConstantInt::get(IntPtrTy, Size.getQuantity()));

clang/lib/CodeGen/CGException.cpp

@@ -390,27 +390,27 @@ namespace {
 void CodeGenFunction::EmitAnyExprToExn(const Expr *e, Address addr) {
   // Make sure the exception object is cleaned up if there's an
   // exception during initialization.
-    pushFullExprCleanup<FreeException>(EHCleanup, addr.getRawPointer(*this));
-    EHScopeStack::stable_iterator cleanup = EHStack.stable_begin();
-
-    // __cxa_allocate_exception returns a void*;  we need to cast this
-    // to the appropriate type for the object.
-    llvm::Type *ty = ConvertTypeForMem(e->getType());
-    Address typedAddr = addr.withElementType(ty);
-
-    // FIXME: this isn't quite right!  If there's a final unelided call
-    // to a copy constructor, then according to [except.terminate]p1 we
-    // must call std::terminate() if that constructor throws, because
-    // technically that copy occurs after the exception expression is
-    // evaluated but before the exception is caught.  But the best way
-    // to handle that is to teach EmitAggExpr to do the final copy
-    // differently if it can't be elided.
-    EmitAnyExprToMem(e, typedAddr, e->getType().getQualifiers(),
-                     /*IsInit*/ true);
-
-    // Deactivate the cleanup block.
-    DeactivateCleanupBlock(
-        cleanup, cast<llvm::Instruction>(typedAddr.getRawPointer(*this)));
+  pushFullExprCleanup<FreeException>(EHCleanup, addr.getRawPointer(*this));
+  EHScopeStack::stable_iterator cleanup = EHStack.stable_begin();
+
+  // __cxa_allocate_exception returns a void*;  we need to cast this
+  // to the appropriate type for the object.
+  llvm::Type *ty = ConvertTypeForMem(e->getType());
+  Address typedAddr = addr.withElementType(ty);
+
+  // FIXME: this isn't quite right!  If there's a final unelided call
+  // to a copy constructor, then according to [except.terminate]p1 we
+  // must call std::terminate() if that constructor throws, because
+  // technically that copy occurs after the exception expression is
+  // evaluated but before the exception is caught.  But the best way
+  // to handle that is to teach EmitAggExpr to do the final copy
+  // differently if it can't be elided.
+  EmitAnyExprToMem(e, typedAddr, e->getType().getQualifiers(),
+                   /*IsInit*/ true);
+
+  // Deactivate the cleanup block.
+  DeactivateCleanupBlock(
+      cleanup, cast<llvm::Instruction>(typedAddr.getRawPointer(*this)));
 }
 
 Address CodeGenFunction::getExceptionSlot() {

clang/lib/CodeGen/CGExprCXX.cpp

@@ -1261,8 +1261,9 @@ void CodeGenFunction::EmitNewArrayInitializer(
 
   // Enter a partial-destruction Cleanup if necessary.
   if (!CleanupDominator && needsEHCleanup(DtorKind)) {
-    pushRegularPartialArrayCleanup(BeginPtr.getRawPointer(*this),
-                                   CurPtr.getRawPointer(*this), ElementType,
+    llvm::Value *BeginPtrRaw = BeginPtr.getRawPointer(*this);
+    llvm::Value *CurPtrRaw = CurPtr.getRawPointer(*this);
+    pushRegularPartialArrayCleanup(BeginPtrRaw, CurPtrRaw, ElementType,
                                    ElementAlign, getDestroyer(DtorKind));
     Cleanup = EHStack.stable_begin();
     CleanupDominator = Builder.CreateUnreachable();

clang/lib/CodeGen/CGOpenMPRuntime.cpp

@@ -4475,12 +4475,12 @@ void CGOpenMPRuntime::emitUpdateClause(CodeGenFunction &CGF, LValue DepobjLVal,
       FlagsLVal);
 
   // Shift the address forward by one element.
-  Address ElementNext =
-      CGF.Builder.CreateConstGEP(Begin, /*Index=*/1, "omp.elementNext");
-  ElementPHI->addIncoming(ElementNext.getRawPointer(CGF),
-                          CGF.Builder.GetInsertBlock());
-  llvm::Value *IsEmpty = CGF.Builder.CreateICmpEQ(
-      ElementNext.getRawPointer(CGF), End, "omp.isempty");
+  llvm::Value *ElementNext =
+      CGF.Builder.CreateConstGEP(Begin, /*Index=*/1, "omp.elementNext")
+          .getRawPointer(CGF);
+  ElementPHI->addIncoming(ElementNext, CGF.Builder.GetInsertBlock());
+  llvm::Value *IsEmpty =
+      CGF.Builder.CreateICmpEQ(ElementNext, End, "omp.isempty");
   CGF.Builder.CreateCondBr(IsEmpty, DoneBB, BodyBB);
   // Done.
   CGF.EmitBlock(DoneBB, /*IsFinished=*/true);
@@ -7304,9 +7304,10 @@ class MappableExprsHandler {
                       CGF.EmitOMPSharedLValue(MC.getAssociatedExpression())
                           .getAddress(CGF);
                 }
-                Size = CGF.Builder.CreatePtrDiff(CGF.Int8Ty,
-                                                 ComponentLB.getRawPointer(CGF),
-                                                 LB.getRawPointer(CGF));
+                llvm::Value *ComponentLBPtr = ComponentLB.getRawPointer(CGF);
+                llvm::Value *LBPtr = LB.getRawPointer(CGF);
+                Size = CGF.Builder.CreatePtrDiff(CGF.Int8Ty, ComponentLBPtr,
+                                                 LBPtr);
                 break;
               }
             }
@@ -7329,9 +7330,10 @@ class MappableExprsHandler {
           CombinedInfo.DevicePtrDecls.push_back(nullptr);
           CombinedInfo.DevicePointers.push_back(DeviceInfoTy::None);
           CombinedInfo.Pointers.push_back(LB.getRawPointer(CGF));
+          llvm::Value *LBPtr = LB.getRawPointer(CGF);
           Size = CGF.Builder.CreatePtrDiff(
               CGF.Int8Ty, CGF.Builder.CreateConstGEP(HB, 1).getRawPointer(CGF),
-              LB.getRawPointer(CGF));
+              LBPtr);
           CombinedInfo.Sizes.push_back(
               CGF.Builder.CreateIntCast(Size, CGF.Int64Ty, /*isSigned=*/true));
           CombinedInfo.Types.push_back(Flags);

clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp

@@ -1511,10 +1511,11 @@ static void shuffleAndStore(CodeGenFunction &CGF, Address SrcAddr,
       Ptr = Address(PhiSrc, Ptr.getElementType(), Ptr.getAlignment());
       ElemPtr =
           Address(PhiDest, ElemPtr.getElementType(), ElemPtr.getAlignment());
-      llvm::Value *PtrDiff =
-          Bld.CreatePtrDiff(CGF.Int8Ty, PtrEnd.getRawPointer(CGF),
-                            Bld.CreatePointerBitCastOrAddrSpaceCast(
-                                Ptr.getRawPointer(CGF), CGF.VoidPtrTy));
+      llvm::Value *PtrEndRaw = PtrEnd.getRawPointer(CGF);
+      llvm::Value *PtrRaw = Ptr.getRawPointer(CGF);
+      llvm::Value *PtrDiff = Bld.CreatePtrDiff(
+          CGF.Int8Ty, PtrEndRaw,
+          Bld.CreatePointerBitCastOrAddrSpaceCast(PtrRaw, CGF.VoidPtrTy));
       Bld.CreateCondBr(Bld.CreateICmpSGT(PtrDiff, Bld.getInt64(IntSize - 1)),
                        ThenBB, ExitBB);
       CGF.EmitBlock(ThenBB);