[OpaquePtrs] Add getNonOpaquePointerElementType() method

[nikic]

Summary

This method is intended for use in places that cannot be reached with opaque pointers, or part of deprecated methods. This makes it easier to see that some uses of getPointerElementType() don't need further action.

I would also like to deprecate PointerType::getElementType() in favor of Type::getPointerElementType() in a followup. The PointerType::getElementType() method is annoying in that you cannot see at a glance whether this is actually accessing a pointer element type, a vector element type or an array element type. I'd like to reduce our TODO list to grep getPointerElementType.

[dexonsmith]

LGTM too; just wanted to share an optional suggestion.

llvm/include/llvm/IR/Type.h

375–378

Given where this should be used, should all uses be required to be documented with a FIXME/TODO/SOMETHING to indicate which of the two cases it is?

I don't feel strongly (up to you), but it might make it easier to audit them.

llvm/lib/AsmParser/LLParser.cpp

1413

E.g., this could have:

// NOTE: getNonOpaquePointerElementType() call unreachable with opaque
// pointers.

[dblaikie]

I /very/ vaguely remember having picked one of "getPointerElementType" and "getElementType" as a sort of intentional transformation like this (though adding a new explicit name's a further step along that direction) back when I started the opaque pointer work to help me classify things I'd cleaned up and things I hadn't... (I think I put an assert(false) in the one I hadn't cleaned up to help me find places I still needed to clean up) but I think it's too far back in my brain.

Happy with whatever you/other folks feels good here.

llvm/include/llvm/IR/Type.h

375–378

Yeah, or possibly have two different function names to self-document those two cases.

[nikic]

llvm/include/llvm/IR/Type.h

375–378

My 2c here is that it doesn't really matter why the method is used, just that the code will get deleted after the migration.

I think it's fairly obvious in most cases, because it's either directly in a deprecated API, or shortly after an isOpaque() check. A comment might be warranted if the check is far removed (e.g. the SROA case is a bit non-obvious, because the opaque pointer check happens in a different function.)

[dblaikie]

llvm/include/llvm/IR/Type.h

375–378

Ah, it sounded like there are two classes here that will need to be treated differently - like the deprecated uses we would expect to cleanup before the opaque pointer flag flip, but the "not reachable" cases would not be cleaned up before that flip? (or the other way around, or some other variation) Is that not the case? Could you expand a bit on the difference between the two cases described by this comment, and the expected lifecycle/path for them?

[nikic]

llvm/include/llvm/IR/Type.h

375–378

In either case, the calls would have to go away when typed pointer support is dropped. For uses in deprecated methods they *might* go away earlier -- this depends on whether we're going to drop C API support in advance or not. I don't think we have a decision on that yet.

[dblaikie]

llvm/include/llvm/IR/Type.h

375–378

Fair enough - thanks for the explanation!

llvm/include/llvm/IR/Type.h

public:
 
   inline uint64_t getArrayNumElements() const;
 
   Type *getArrayElementType() const {
     assert(getTypeID() == ArrayTyID);
     return ContainedTys[0];
   }
 
+  /// This method is deprecated without replacement. Pointer element types are
+  /// not available with opaque pointers.
   Type *getPointerElementType() const {
+    return getNonOpaquePointerElementType();
+  }
+
+  /// Only use this method in code that is not reachable with opaque pointers,
+  /// or part of deprecated methods that will be removed as part of the opaque
+  /// pointers transition.
+  Type *getNonOpaquePointerElementType() const {
     assert(getTypeID() == PointerTyID);
     assert(NumContainedTys &&
            "Attempting to get element type of opaque pointer");
     return ContainedTys[0];
   }
 
   /// Given vector type, change the element type,
   /// whilst keeping the old number of elements.

llvm/lib/AsmParser/LLParser.cpp

static inline GlobalValue *createGlobalFwdRef(Module *M, PointerType *PTy) {
   // For opaque pointers, the used global type does not matter. We will later
   // RAUW it with a global/function of the correct type.
   if (PTy->isOpaque())
     return new GlobalVariable(*M, Type::getInt8Ty(M->getContext()), false,
                               GlobalValue::ExternalWeakLinkage, nullptr, "",
                               nullptr, GlobalVariable::NotThreadLocal,
                               PTy->getAddressSpace());
 
-  if (auto *FT = dyn_cast<FunctionType>(PTy->getPointerElementType()))
+  Type *ElemTy = PTy->getNonOpaquePointerElementType();
+  if (auto *FT = dyn_cast<FunctionType>(ElemTy))
     return Function::Create(FT, GlobalValue::ExternalWeakLinkage,
                             PTy->getAddressSpace(), "", M);
   else
-    return new GlobalVariable(*M, PTy->getPointerElementType(), false,
+    return new GlobalVariable(*M, ElemTy, false,
                               GlobalValue::ExternalWeakLinkage, nullptr, "",
                               nullptr, GlobalVariable::NotThreadLocal,
                               PTy->getAddressSpace());
 }
 
 Value *LLParser::checkValidVariableType(LocTy Loc, const Twine &Name, Type *Ty,
                                         Value *Val) {
   Type *ValTy = Val->getType();
bool LLParser::parseFunctionHeader(Function *&Fn, bool IsDefine) {
   GlobalValue *FwdFn = nullptr;
   if (!FunctionName.empty()) {
     // If this was a definition of a forward reference, remove the definition
     // from the forward reference table and fill in the forward ref.
     auto FRVI = ForwardRefVals.find(FunctionName);
     if (FRVI != ForwardRefVals.end()) {
       FwdFn = FRVI->second.first;
       if (!FwdFn->getType()->isOpaque()) {
-        if (!FwdFn->getType()->getPointerElementType()->isFunctionTy())
+        if (!FwdFn->getType()->getNonOpaquePointerElementType()->isFunctionTy())
           return error(FRVI->second.second, "invalid forward reference to "
                                             "function as global value!");
         if (FwdFn->getType() != PFT)
           return error(FRVI->second.second,
                        "invalid forward reference to "
                        "function '" +
                            FunctionName +
                            "' with wrong type: "

llvm/lib/IR/Core.cpp

return wrap(ConstantExpr::getAShr(unwrap<Constant>(LHSConstant),
                                     unwrap<Constant>(RHSConstant)));
 }
 
 LLVMValueRef LLVMConstGEP(LLVMValueRef ConstantVal,
                           LLVMValueRef *ConstantIndices, unsigned NumIndices) {
   ArrayRef<Constant *> IdxList(unwrap<Constant>(ConstantIndices, NumIndices),
                                NumIndices);
   Constant *Val = unwrap<Constant>(ConstantVal);
-  Type *Ty =
-      cast<PointerType>(Val->getType()->getScalarType())->getElementType();
+  Type *Ty = Val->getType()->getScalarType()->getNonOpaquePointerElementType();
   return wrap(ConstantExpr::getGetElementPtr(Ty, Val, IdxList));
 }
 
 LLVMValueRef LLVMConstGEP2(LLVMTypeRef Ty, LLVMValueRef ConstantVal,
                            LLVMValueRef *ConstantIndices, unsigned NumIndices) {
   ArrayRef<Constant *> IdxList(unwrap<Constant>(ConstantIndices, NumIndices),
                                NumIndices);
   Constant *Val = unwrap<Constant>(ConstantVal);
   return wrap(ConstantExpr::getGetElementPtr(unwrap(Ty), Val, IdxList));
 }
 
 LLVMValueRef LLVMConstInBoundsGEP(LLVMValueRef ConstantVal,
                                   LLVMValueRef *ConstantIndices,
                                   unsigned NumIndices) {
   ArrayRef<Constant *> IdxList(unwrap<Constant>(ConstantIndices, NumIndices),
                                NumIndices);
   Constant *Val = unwrap<Constant>(ConstantVal);
-  Type *Ty =
-      cast<PointerType>(Val->getType()->getScalarType())->getElementType();
+  Type *Ty = Val->getType()->getScalarType()->getNonOpaquePointerElementType();
   return wrap(ConstantExpr::getInBoundsGetElementPtr(Ty, Val, IdxList));
 }
 
 LLVMValueRef LLVMConstInBoundsGEP2(LLVMTypeRef Ty, LLVMValueRef ConstantVal,
                                    LLVMValueRef *ConstantIndices,
                                    unsigned NumIndices) {
   ArrayRef<Constant *> IdxList(unwrap<Constant>(ConstantIndices, NumIndices),
                                NumIndices);
void LLVMSetExternallyInitialized(LLVMValueRef GlobalVar, LLVMBool IsExtInit) {
   unwrap<GlobalVariable>(GlobalVar)->setExternallyInitialized(IsExtInit);
 }
 
 /*--.. Operations on aliases ......................................--*/
 
 LLVMValueRef LLVMAddAlias(LLVMModuleRef M, LLVMTypeRef Ty, LLVMValueRef Aliasee,
                           const char *Name) {
   auto *PTy = cast<PointerType>(unwrap(Ty));
-  return wrap(GlobalAlias::create(PTy->getElementType(), PTy->getAddressSpace(),
+  return wrap(GlobalAlias::create(PTy->getNonOpaquePointerElementType(),
+                                  PTy->getAddressSpace(),
                                   GlobalValue::ExternalLinkage, Name,
                                   unwrap<Constant>(Aliasee), unwrap(M)));
 }
 
 LLVMValueRef LLVMAddAlias2(LLVMModuleRef M, LLVMTypeRef ValueTy,
                            unsigned AddrSpace, LLVMValueRef Aliasee,
                            const char *Name) {
   return wrap(GlobalAlias::create(unwrap(ValueTy), AddrSpace,

 }
 
 LLVMValueRef LLVMBuildInvoke(LLVMBuilderRef B, LLVMValueRef Fn,
                              LLVMValueRef *Args, unsigned NumArgs,
                              LLVMBasicBlockRef Then, LLVMBasicBlockRef Catch,
                              const char *Name) {
   Value *V = unwrap(Fn);
   FunctionType *FnT =
-      cast<FunctionType>(cast<PointerType>(V->getType())->getElementType());
+      cast<FunctionType>(V->getType()->getNonOpaquePointerElementType());
 
   return wrap(
       unwrap(B)->CreateInvoke(FnT, unwrap(Fn), unwrap(Then), unwrap(Catch),
                               makeArrayRef(unwrap(Args), NumArgs), Name));
 }
 
 LLVMValueRef LLVMBuildInvoke2(LLVMBuilderRef B, LLVMTypeRef Ty, LLVMValueRef Fn,
                               LLVMValueRef *Args, unsigned NumArgs,
return wrap(unwrap(B)->Insert(
      CallInst::CreateFree(unwrap(PointerVal), unwrap(B)->GetInsertBlock())));
 }
 
 LLVMValueRef LLVMBuildLoad(LLVMBuilderRef B, LLVMValueRef PointerVal,
                            const char *Name) {
   Value *V = unwrap(PointerVal);
   PointerType *Ty = cast<PointerType>(V->getType());
 
-  return wrap(unwrap(B)->CreateLoad(Ty->getElementType(), V, Name));
+  return wrap(unwrap(B)->CreateLoad(Ty->getNonOpaquePointerElementType(), V,
+                                    Name));
 }
 
 LLVMValueRef LLVMBuildLoad2(LLVMBuilderRef B, LLVMTypeRef Ty,
                             LLVMValueRef PointerVal, const char *Name) {
   return wrap(unwrap(B)->CreateLoad(unwrap(Ty), unwrap(PointerVal), Name));
 }
 
 LLVMValueRef LLVMBuildStore(LLVMBuilderRef B, LLVMValueRef Val,
unwrap(B)->CreateFence(mapFromLLVMOrdering(Ordering),
                            Name));
 }
 
 LLVMValueRef LLVMBuildGEP(LLVMBuilderRef B, LLVMValueRef Pointer,
                           LLVMValueRef *Indices, unsigned NumIndices,
                           const char *Name) {
   ArrayRef<Value *> IdxList(unwrap(Indices), NumIndices);
   Value *Val = unwrap(Pointer);
-  Type *Ty =
-      cast<PointerType>(Val->getType()->getScalarType())->getElementType();
+  Type *Ty = Val->getType()->getScalarType()->getNonOpaquePointerElementType();
   return wrap(unwrap(B)->CreateGEP(Ty, Val, IdxList, Name));
 }
 
 LLVMValueRef LLVMBuildGEP2(LLVMBuilderRef B, LLVMTypeRef Ty,
                            LLVMValueRef Pointer, LLVMValueRef *Indices,
                            unsigned NumIndices, const char *Name) {
   ArrayRef<Value *> IdxList(unwrap(Indices), NumIndices);
   return wrap(unwrap(B)->CreateGEP(unwrap(Ty), unwrap(Pointer), IdxList, Name));
 }
 
 LLVMValueRef LLVMBuildInBoundsGEP(LLVMBuilderRef B, LLVMValueRef Pointer,
                                   LLVMValueRef *Indices, unsigned NumIndices,
                                   const char *Name) {
   ArrayRef<Value *> IdxList(unwrap(Indices), NumIndices);
   Value *Val = unwrap(Pointer);
-  Type *Ty =
-      cast<PointerType>(Val->getType()->getScalarType())->getElementType();
+  Type *Ty = Val->getType()->getScalarType()->getNonOpaquePointerElementType();
   return wrap(unwrap(B)->CreateInBoundsGEP(Ty, Val, IdxList, Name));
 }
 
 LLVMValueRef LLVMBuildInBoundsGEP2(LLVMBuilderRef B, LLVMTypeRef Ty,
                                    LLVMValueRef Pointer, LLVMValueRef *Indices,
                                    unsigned NumIndices, const char *Name) {
   ArrayRef<Value *> IdxList(unwrap(Indices), NumIndices);
   return wrap(
       unwrap(B)->CreateInBoundsGEP(unwrap(Ty), unwrap(Pointer), IdxList, Name));
 }
 
 LLVMValueRef LLVMBuildStructGEP(LLVMBuilderRef B, LLVMValueRef Pointer,
                                 unsigned Idx, const char *Name) {
   Value *Val = unwrap(Pointer);
-  Type *Ty =
-      cast<PointerType>(Val->getType()->getScalarType())->getElementType();
+  Type *Ty = Val->getType()->getScalarType()->getNonOpaquePointerElementType();
   return wrap(unwrap(B)->CreateStructGEP(Ty, Val, Idx, Name));
 }
 
 LLVMValueRef LLVMBuildStructGEP2(LLVMBuilderRef B, LLVMTypeRef Ty,
                                  LLVMValueRef Pointer, unsigned Idx,
                                  const char *Name) {
   return wrap(
       unwrap(B)->CreateStructGEP(unwrap(Ty), unwrap(Pointer), Idx, Name));
LLVMValueRef LLVMBuildPhi(LLVMBuilderRef B, LLVMTypeRef Ty, const char *Name) {
   return wrap(unwrap(B)->CreatePHI(unwrap(Ty), 0, Name));
 }
 
 LLVMValueRef LLVMBuildCall(LLVMBuilderRef B, LLVMValueRef Fn,
                            LLVMValueRef *Args, unsigned NumArgs,
                            const char *Name) {
   Value *V = unwrap(Fn);
   FunctionType *FnT =
-      cast<FunctionType>(cast<PointerType>(V->getType())->getElementType());
+      cast<FunctionType>(V->getType()->getNonOpaquePointerElementType());
 
   return wrap(unwrap(B)->CreateCall(FnT, unwrap(Fn),
                                     makeArrayRef(unwrap(Args), NumArgs), Name));
 }
 
 LLVMValueRef LLVMBuildCall2(LLVMBuilderRef B, LLVMTypeRef Ty, LLVMValueRef Fn,
                             LLVMValueRef *Args, unsigned NumArgs,
                             const char *Name) {

llvm/lib/Transforms/Coroutines/Coroutines.cpp

 static void checkAsyncFuncPointer(const Instruction *I, Value *V) {
   auto *AsyncFuncPtrAddr = dyn_cast<GlobalVariable>(V->stripPointerCasts());
   if (!AsyncFuncPtrAddr)
     fail(I, "llvm.coro.id.async async function pointer not a global", V);
 
   if (AsyncFuncPtrAddr->getType()->isOpaquePointerTy())
     return;
 
-  auto *StructTy =
-      cast<StructType>(AsyncFuncPtrAddr->getType()->getPointerElementType());
+  auto *StructTy = cast<StructType>(
+      AsyncFuncPtrAddr->getType()->getNonOpaquePointerElementType());
   if (StructTy->isOpaque() || !StructTy->isPacked() ||
       StructTy->getNumElements() != 2 ||
       !StructTy->getElementType(0)->isIntegerTy(32) ||
       !StructTy->getElementType(1)->isIntegerTy(32))
     fail(I,
          "llvm.coro.id.async async function pointer argument's type is not "
          "<{i32, i32}>",
          V);

llvm/lib/Transforms/Scalar/SROA.cpp

static Value *buildGEP(IRBuilderTy &IRB, Value *BasePtr,
   if (Indices.empty())
     return BasePtr;
 
   // A single zero index is a no-op, so check for this and avoid building a GEP
   // in that case.
   if (Indices.size() == 1 && cast<ConstantInt>(Indices.back())->isZero())
     return BasePtr;
 
-  return IRB.CreateInBoundsGEP(BasePtr->getType()->getPointerElementType(),
-                               BasePtr, Indices, NamePrefix + "sroa_idx");
+  return IRB.CreateInBoundsGEP(
+      BasePtr->getType()->getNonOpaquePointerElementType(), BasePtr, Indices,
+      NamePrefix + "sroa_idx");
 }
 
 /// Get a natural GEP off of the BasePtr walking through Ty toward
 /// TargetTy without changing the offset of the pointer.
 ///
 /// This routine assumes we've already established a properly offset GEP with
 /// Indices, and arrived at the Ty type. The goal is to continue to GEP with
 /// zero-indices down through type layers until we find one the same as