[PATCH] D152321: [clang] Replace use of Type::getPointerTo() (NFC)

[Youngsuk Kim via Phabricator via cfe-commits]

JOE1994 added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuilder.h:170
   Address CreateElementBitCast(Address Addr, llvm::Type *Ty,
                                const llvm::Twine &Name = "") {
+    return Address(Addr.getPointer(), Ty,
----------------
barannikov88 wrote:
> The argument can be removed.
> 
> Idea for a follow-up: I would also consider removing this method because it does not do what its name says.
> Maybe replace it with `Address::withElementType` analagous to `Address::withPointer` / `Address::withAlignment`?
> 
Thank you for the suggestion.

I have a follow-up reivision for removing the `Name` parameter. (https://reviews.llvm.org/D152551)

After this revision gets merged, I can update the follow-up revision to replace `CreateElementBitCast` also.


================
Comment at: clang/lib/CodeGen/CGCUDANV.cpp:238-240
   CharPtrTy = llvm::PointerType::getUnqual(Types.ConvertType(Ctx.CharTy));
   VoidPtrTy = cast<llvm::PointerType>(Types.ConvertType(Ctx.VoidPtrTy));
+  VoidPtrPtrTy = llvm::PointerType::get(CGM.getLLVMContext(), 0);
----------------
barannikov88 wrote:
> These are all the same types. Replace the variables with single `PtrTy`?
> 
Thank you for the suggestion.

Unifying them to a single `PtrTy` adds a lot of diff,
so I think it deserves to be in a separate standalone commit.
(I can create a follow-up revision after this gets merged).


================
Comment at: clang/lib/CodeGen/CGCXX.cpp:175
   // Create the alias with no name.
+  llvm::Type *AliasValueType = getTypes().GetFunctionType(AliasDecl);
   auto *Alias = llvm::GlobalAlias::create(AliasValueType, 0, Linkage, "",
----------------
barannikov88 wrote:
> This looks wrong. It used to be `GetFunctionType(TargetDecl)`.
> 
I've simply moved the line defining `AliasValueType` closer to its use.
I think `AliasValueType` is right to be initialized with `getFunctionType(AliasDecl)`.

Moving the line to below isn't necessary, and it's rather causing confusion.
I'll move it back to where it was.


================
Comment at: clang/lib/CodeGen/CGCXX.cpp:184
   if (Entry) {
-    assert(Entry->getType() == AliasType &&
+    assert(Entry->getValueType() == AliasValueType &&
+           Entry->getAddressSpace() == Alias->getAddressSpace() &&
----------------
barannikov88 wrote:
> What's the reason for this change?
Since `AliasType` got removed above, the assert condition needs to be updated one way or another.

[The assert was written in 2010 (before opaque pointers)](https://github.com/llvm/llvm-project/commit/aea181de04d8be743db1629e0c054abff68500c6),
and it's checking equality of two pointer types (which was equivalent to checking equality of pointee type & address space).

With opaque pointers enabled, I thought the assert needs to be updated to check equality of both the pointee types & address spaces separately in order to preserve the original intent.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152321/new/

https://reviews.llvm.org/D152321