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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits