[OpaquePtr] Support call instruction

[nikic]

Summary

Add support for call of opaque pointer, currently only possible for indirect calls.

[arsenm]

llvm/lib/AsmParser/LLParser.cpp

1479–1480

This comment doesn't make sense, it reads like the address space and opaque pointers are mutually exclusive things

[aeubanks]

could you add a test for a call with arguments?

llvm/lib/AsmParser/LLParser.cpp

1480

we still need to check the address space like below

[nikic]

Preserve address space check, add test with argument.

While doing the latter, I found out that

define void @call(void (...)* %p) {
  call void (...) %p(void (...)* %p)
  ret void
}

fails verify-uselistorder (independently of this patch). I happened to use that as the first thing I tried :/

[aeubanks]

llvm/lib/AsmParser/LLParser.cpp

1482

if we have an opaque pointer of a wrong address space, we could get past the first CheckType(), then call getElementType() on the opaque pointer type below

[nikic]

Use getWithSamePointeeType().

[nikic]

llvm/lib/AsmParser/LLParser.cpp

1482

I've changed it to use getWithSamePointeeType() instead. It's not possible to actually trigger this case right now, because Ty currently can't be opaque (would need D103503). Only ValTy can be opaque.

[nikic]

Rebase

[aeubanks]

at some point, this might not be necessary if all function pointers become opaque pointers, right?
but for now we do need this

[nikic]

In D104740#2836318, @aeubanks wrote:

at some point, this might not be necessary if all function pointers become opaque pointers, right?
but for now we do need this

Yeah, the LLParser part wouldn't be needed with opaque function pointers.

[dexonsmith]

In D104740#2834433, @nikic wrote:

Preserve address space check, add test with argument.

While doing the latter, I found out that

define void @call(void (...)* %p) {
  call void (...) %p(void (...)* %p)
  ret void
}

fails verify-uselistorder (independently of this patch). I happened to use that as the first thing I tried :/

Interesting; given that there are two uses of %p maybe one of them isn't being tracked. Did you investigate?

[dexonsmith]

In D104740#2836407, @dexonsmith wrote:

In D104740#2834433, @nikic wrote:

Preserve address space check, add test with argument.

While doing the latter, I found out that

define void @call(void (...)* %p) {
  call void (...) %p(void (...)* %p)
  ret void
}

fails verify-uselistorder (independently of this patch). I happened to use that as the first thing I tried :/

Interesting; given that there are two uses of %p maybe one of them isn't being tracked. Did you investigate?

Looks like it's caused by this:

void CallInst::init(FunctionType *FTy, Value *Func, ArrayRef<Value *> Args,
                    ArrayRef<OperandBundleDef> Bundles, const Twine &NameStr) {
  // ...

  setCalledOperand(Func);

  // ...

  llvm::copy(Args, op_begin());

  // ...
}

and

/// The last operand is the called operand.
static constexpr int CalledOperandOpEndIdx = -1;
void setCalledOperand(Value *V) { Op<CalledOperandOpEndIdx>() = V; }

The use-list order "predictors" in BitcodeWriter and AsmWriter assume that operands will be initialized in order. Moving the call to setCalledOperand after the llvm::copy fixes it.

I'll look for other similar bugs and post a patch.

[dexonsmith]

In D104740#2836529, @dexonsmith wrote:

I'll look for other similar bugs and post a patch.

Posted https://reviews.llvm.org/D104805. Once that lands, verify-uselistorder should work for your original testcase.

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 */) {
-  if (Val->getType() == Ty)
+                                        Value *Val, bool IsCall) {
+  Type *ValTy = Val->getType();
+  if (ValTy == Ty)
+    return Val;
+  // For calls, we also allow opaque pointers.
+  if (IsCall && ValTy == PointerType::get(Ty->getContext(),
+                                          Ty->getPointerAddressSpace()))
     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(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

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

case bitc::FUNC_CODE_INST_CALL: {
       PointerType *OpTy = dyn_cast<PointerType>(Callee->getType());
       if (!OpTy)
         return error("Callee is not a pointer type");
       if (!FTy) {
         FTy = dyn_cast<FunctionType>(
             cast<PointerType>(Callee->getType())->getElementType());
         if (!FTy)
           return error("Callee is not of pointer to function type");
-      } else if (cast<PointerType>(Callee->getType())->getElementType() != FTy)
+      } else if (!OpTy->isOpaqueOrPointeeTypeMatches(FTy))
         return error("Explicit call type does not match pointee type of "
                      "callee operand");
       if (Record.size() < FTy->getNumParams() + OpNum)
         return error("Insufficient operands to call");
 
       SmallVector<Value*, 16> Args;
       SmallVector<Type *, 16> ArgsTys;
       // Read the fixed params.

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

for (const BasicBlock &BB : F)
 
           EnumerateMetadata(&F, MD->getMetadata());
         }
         if (auto *SVI = dyn_cast<ShuffleVectorInst>(&I))
           EnumerateType(SVI->getShuffleMaskForBitcode()->getType());
         if (auto *GEP = dyn_cast<GetElementPtrInst>(&I))
           EnumerateType(GEP->getSourceElementType());
         EnumerateType(I.getType());
-        if (const auto *Call = dyn_cast<CallBase>(&I))
+        if (const auto *Call = dyn_cast<CallBase>(&I)) {
           EnumerateAttributes(Call->getAttributes());
+          EnumerateType(Call->getFunctionType());
+        }
 
         // Enumerate metadata attached with this instruction.
         MDs.clear();
         I.getAllMetadataOtherThanDebugLoc(MDs);
         for (unsigned i = 0, e = MDs.size(); i != e; ++i)
           EnumerateMetadata(&F, MDs[i].second);
 
         // Don't enumerate the location directly -- it has a special record

llvm/lib/IR/Verifier.cpp

void Verifier::visitPHINode(PHINode &PN) {
   visitInstruction(PN);
 }
 
 void Verifier::visitCallBase(CallBase &Call) {
   Assert(Call.getCalledOperand()->getType()->isPointerTy(),
          "Called function must be a pointer!", Call);
   PointerType *FPTy = cast<PointerType>(Call.getCalledOperand()->getType());
 
-  Assert(FPTy->getElementType()->isFunctionTy(),
-         "Called function is not pointer to function type!", Call);
-
-  Assert(FPTy->getElementType() == Call.getFunctionType(),
+  Assert(FPTy->isOpaqueOrPointeeTypeMatches(Call.getFunctionType()),
          "Called function is not the same type as the call!", Call);
 
   FunctionType *FTy = Call.getFunctionType();
 
   // Verify that the correct number of arguments are being passed
   if (FTy->isVarArg())
     Assert(Call.arg_size() >= FTy->getNumParams(),
            "Called function requires more parameters than were provided!",

llvm/test/Assembler/opaque-ptr.ll

 
 ; CHECK: define void @atomicrmw(ptr %a, i32 %i)
 ; CHECK:     %b = atomicrmw add ptr %a, i32 %i acquire
 ; CHECK:     ret void
 define void @atomicrmw(ptr %a, i32 %i) {
     %b = atomicrmw add ptr %a, i32 %i acquire
     ret void
 }
+
+; CHECK: define void @call(ptr %p)
+; CHECK:     call void %p()
+; CHECK:     ret void
+define void @call(ptr %p) {
+  call void %p()
+  ret void
+}
+
+; CHECK: define void @call_arg(ptr %p, i32 %a)
+; CHECK:     call void %p(i32 %a)
+; CHECK:     ret void
+define void @call_arg(ptr %p, i32 %a) {
+  call void %p(i32 %a)
+  ret void
+}