[LLParser] Remove special handling for call address space

[nikic]

Summary

Spin-off from D104740: I don't think this special handling is needed at all. Calls in textual IR are annotated with addrspace(N) (which defaults to the program address space from data layout) and specifies the expected pointer address space of the callee. There is no need to special-case the program address space on top of that, as it already is the default expected address space, and we shouldn't allow use of the program address space if the call was explicitly annotated with some other address space.

No tests fail with this special handling removed.

[aeubanks]

I'm confused by "The pointer address space of the callee should always match the specified addrspace". The addrspace on the call sounds like it's basically the "expected" addrspace and could affect lower of the call instruction itself. But the call operand might not be of that addrspace, whether it's a direct call's callee, or an indirect call's function pointer.

llvm/lib/AsmParser/LLParser.cpp

1472

IsCall isn't being used anymore

[nikic]

In D104752#2834690, @aeubanks wrote:

I'm confused by "The pointer address space of the callee should always match the specified addrspace". The addrspace on the call sounds like it's basically the "expected" addrspace and could affect lower of the call instruction itself. But the call operand might not be of that addrspace, whether it's a direct call's callee, or an indirect call's function pointer.

LangRef says:

The optional addrspace attribute can be used to indicate the address space of the called function. If it is not specified, the program address space from the datalayout string will be used.

My reading here is that the call addrspace and the callee addrspace must be the same.

Looking back over D47541, I think this code is an artifact from an earlier version of the patch that did not have addrspace on calls.

llvm/lib/AsmParser/LLParser.cpp

1472

It will be used by D104740 again, so I don't want to remove the parameter. I can comment the name though.

[nikic]

Comment temporarily unused parameter name to make sure it doesn't warn.

One more thing to note here, because it probably wasn't clear from my original description: The call addrspace is a pure textural IR concept and is only used to determine the expected address space of the callee pointer type. There is no concept of a call addrspace for in-memory IR, just the addrspace of the callee pointer type.

[arichardson]

In D104752#2834718, @nikic wrote:

In D104752#2834690, @aeubanks wrote:

I'm confused by "The pointer address space of the callee should always match the specified addrspace". The addrspace on the call sounds like it's basically the "expected" addrspace and could affect lower of the call instruction itself. But the call operand might not be of that addrspace, whether it's a direct call's callee, or an indirect call's function pointer.

LangRef says:

The optional addrspace attribute can be used to indicate the address space of the called function. If it is not specified, the program address space from the datalayout string will be used.

My reading here is that the call addrspace and the callee addrspace must be the same.

Looking back over D47541, I think this code is an artifact from an earlier version of the patch that did not have addrspace on calls.

It's been a long time since I looked at that code, but it was originally needed to avoid adding addrspace(200) to every call instruction. I just tried removing this from our downstream CHERI LLParser.cpp and all tests are still passing so it is clearly no longer needed.

[nikic]

In D104752#2835466, @arichardson wrote:

It's been a long time since I looked at that code, but it was originally needed to avoid adding addrspace(200) to every call instruction. I just tried removing this from our downstream CHERI LLParser.cpp and all tests are still passing so it is clearly no longer needed.

Thanks for checking!

llvm/lib/AsmParser/LLParser.cpp

static inline GlobalValue *createGlobalFwdRef(Module *M, PointerType *PTy,
   else
     return new GlobalVariable(*M, PTy->getElementType(), false,
                               GlobalValue::ExternalWeakLinkage, nullptr, Name,
                               nullptr, GlobalVariable::NotThreadLocal,
                               PTy->getAddressSpace());
 }
 
 Value *LLParser::checkValidVariableType(LocTy Loc, const Twine &Name, Type *Ty,
-                                        Value *Val, bool IsCall) {
+                                        Value *Val, bool /* IsCall */) {
   if (Val->getType() == Ty)
     return Val;
-  // For calls we also accept variables in the program address space.
-  Type *SuggestedTy = Ty;
-  if (IsCall && isa<PointerType>(Ty)) {
-    Type *TyInProgAS = cast<PointerType>(Ty)->getElementType()->getPointerTo(
-        M->getDataLayout().getProgramAddressSpace());
-    SuggestedTy = TyInProgAS;
-    if (Val->getType() == TyInProgAS)
-      return Val;
-  }
   if (Ty->isLabelTy())
     error(Loc, "'" + Name + "' is not a basic block");
   else
     error(Loc, "'" + Name + "' defined with type '" +
                    getTypeString(Val->getType()) + "' but expected '" +
-                   getTypeString(SuggestedTy) + "'");
+                   getTypeString(Ty) + "'");
   return nullptr;
 }
 
 /// getGlobalVal - Get a value with the specified name or ID, creating a
 /// forward reference record if needed.  This can return null if the value
 /// exists but does not have the right type.
 GlobalValue *LLParser::getGlobalVal(const std::string &Name, Type *Ty,
                                     LocTy Loc, bool IsCall) {