[LangRef] Require elementtype attribute for indirect inline asm operands

[nikic]

Summary

Indirect inline asm operands may require the materialization of a memory access according to the pointer element type. As this will no longer be available with opaque pointers, we require it to be explicitly annotated using the elementtype attribute, for example:

define void @test(i32* %p, i32 %x) {
  call void asm "addl $1, $0", "=*rm,r"(i32* elementtype(i32) %p, i32 %x)
  ret void
}

This patch only includes the LangRef change and Verifier updates to allow adding the elementtype attribute in this position. It does not yet enforce this, as this will require changes on the clang side (and test updates) first.

Something I'm a bit unsure about is whether we really need the elementtype for all indirect constraints, rather than only indirect register constraints. I think indirect memory constraints might not need it (though the backend code is written in a way that does require it). I think it's okay to just make this a general requirement though, as this means we don't need to carefully deal with multiple or alternative constraints. In addition, I believe that MemorySanitizer benefits from having the element type even in cases where it may not be strictly necessary for normal lowering (https://github.com/llvm/llvm-project/blob/cd2b050fa4995b75b9c36fae16c0d9f105b67585/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp#L4066).

[lebedev.ri]

Seems fine to me.
Thanks.

llvm/test/Verifier/inline-asm-indirect-operand.ll

16–21

Worth testing that the checking is done for call/invoke/asm goto, not just call?

[jrtc27]

Will there be an auto upgrade for this if a non-opaque pointer is used?

[nikic]

In D116531#3222447, @jrtc27 wrote:

Will there be an auto upgrade for this if a non-opaque pointer is used?

Yes, old bitcode will be auto-upgraded.

llvm/docs/LangRef.rst

 It is also possible to use an indirect *register* constraint, but only on output
 (e.g. "``=*r``"). This will cause LLVM to allocate a register for an output
 value normally, and then, separately emit a store to the address provided as
 input, after the provided inline asm. (It's not clear what value this
 functionality provides, compared to writing the store explicitly after the asm
 statement, and it can only produce worse code, since it bypasses many
 optimization passes. I would recommend not using it.)
 
+Call arguments for indirect constraints must have pointer type and must specify
+the :ref:`elementtype <attr_elementtype>` attribute to indicate the pointer
+element type.
 
 Clobber constraints
 """""""""""""""""""
 
 A clobber constraint is indicated by a "``~``" prefix. A clobber does not
 consume an input operand, nor generate an output. Clobbers cannot use any of the
 general constraint code letters -- they may use only explicit register
 constraints, e.g. "``~{eax}``". The one exception is that a clobber string of

llvm/lib/IR/Verifier.cpp

#include "llvm/IR/Metadata.def"
   void verifyTailCCMustTailAttrs(const AttrBuilder &Attrs, StringRef Context);
   void verifyMustTailCall(CallInst &CI);
   bool verifyAttributeCount(AttributeList Attrs, unsigned Params);
   void verifyAttributeTypes(AttributeSet Attrs, const Value *V);
   void verifyParameterAttrs(AttributeSet Attrs, Type *Ty, const Value *V);
   void checkUnsignedBaseTenFuncAttr(AttributeList Attrs, StringRef Attr,
                                     const Value *V);
   void verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
-                           const Value *V, bool IsIntrinsic);
+                           const Value *V, bool IsIntrinsic, bool IsInlineAsm);
   void verifyFunctionMetadata(ArrayRef<std::pair<unsigned, MDNode *>> MDs);
 
   void visitConstantExprsRecursively(const Constant *EntryC);
   void visitConstantExpr(const ConstantExpr *CE);
+  void verifyInlineAsmCall(const CallBase &Call);
   void verifyStatepoint(const CallBase &Call);
   void verifyFrameRecoverIndices();
   void verifySiblingFuncletUnwinds();
 
   void verifyFragmentExpression(const DbgVariableIntrinsic &I);
   template <typename ValueOrMetadata>
   void verifyFragmentExpression(const DIVariable &V,
                                 DIExpression::FragmentInfo Fragment,
if (Attrs.hasFnAttr(Attr)) {
     if (S.getAsInteger(10, N))
       CheckFailed("\"" + Attr + "\" takes an unsigned integer: " + S, V);
   }
 }
 
 // Check parameter attributes against a function type.
 // The value V is printed in error messages.
 void Verifier::verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
-                                   const Value *V, bool IsIntrinsic) {
+                                   const Value *V, bool IsIntrinsic,
+                                   bool IsInlineAsm) {
   if (Attrs.isEmpty())
     return;
 
   if (AttributeListsVisited.insert(Attrs.getRawPointer()).second) {
     Assert(Attrs.hasParentContext(Context),
            "Attribute list does not match Module context!", &Attrs, V);
     for (const auto &AttrSet : Attrs) {
       Assert(!AttrSet.hasAttributes() || AttrSet.hasParentContext(Context),
void Verifier::verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
   // Verify parameter attributes.
   for (unsigned i = 0, e = FT->getNumParams(); i != e; ++i) {
     Type *Ty = FT->getParamType(i);
     AttributeSet ArgAttrs = Attrs.getParamAttrs(i);
 
     if (!IsIntrinsic) {
       Assert(!ArgAttrs.hasAttribute(Attribute::ImmArg),
              "immarg attribute only applies to intrinsics",V);
+      if (!IsInlineAsm)
         Assert(!ArgAttrs.hasAttribute(Attribute::ElementType),
-             "Attribute 'elementtype' can only be applied to intrinsics.", V);
+               "Attribute 'elementtype' can only be applied to intrinsics"
+               " and inline asm.", V);
     }
 
     verifyParameterAttrs(ArgAttrs, Ty, V);
 
     if (ArgAttrs.hasAttribute(Attribute::Nest)) {
       Assert(!SawNest, "More than one parameter has attribute nest!", V);
       SawNest = true;
     }

 }
 
 bool Verifier::verifyAttributeCount(AttributeList Attrs, unsigned Params) {
   // There shouldn't be more attribute sets than there are parameters plus the
   // function and return value.
   return Attrs.getNumAttrSets() <= Params + 2;
 }
 
+void Verifier::verifyInlineAsmCall(const CallBase &Call) {
+  const InlineAsm *IA = cast<InlineAsm>(Call.getCalledOperand());
+  unsigned ArgNo = 0;
+  for (const InlineAsm::ConstraintInfo &CI : IA->ParseConstraints()) {
+    // Only deal with constraints that correspond to call arguments.
+    bool HasArg = CI.Type == InlineAsm::isInput ||
+                  (CI.Type == InlineAsm::isOutput && CI.isIndirect);
+    if (!HasArg)
+      continue;
+
+    if (CI.isIndirect) {
+      const Value *Arg = Call.getArgOperand(ArgNo);
+      Assert(Arg->getType()->isPointerTy(),
+             "Operand for indirect constraint must have pointer type",
+             &Call);
+
+      // TODO: Require elementtype attribute here.
+    } else {
+      Assert(!Call.paramHasAttr(ArgNo, Attribute::ElementType),
+             "Elementtype attribute can only be applied for indirect "
+             "constraints", &Call);
+    }
+
+    ArgNo++;
+  }
+}
+
 /// Verify that statepoint intrinsic is well formed.
 void Verifier::verifyStatepoint(const CallBase &Call) {
   assert(Call.getCalledFunction() &&
          Call.getCalledFunction()->getIntrinsicID() ==
              Intrinsic::experimental_gc_statepoint);
 
   Assert(!Call.doesNotAccessMemory() && !Call.onlyReadsMemory() &&
              !Call.onlyAccessesArgMemory(),
void Verifier::visitFunction(const Function &F) {
   AttributeList Attrs = F.getAttributes();
 
   Assert(verifyAttributeCount(Attrs, FT->getNumParams()),
          "Attribute after last parameter!", &F);
 
   bool IsIntrinsic = F.isIntrinsic();
 
   // Check function attributes.
-  verifyFunctionAttrs(FT, Attrs, &F, IsIntrinsic);
+  verifyFunctionAttrs(FT, Attrs, &F, IsIntrinsic, /* IsInlineAsm */ false);
 
   // On function declarations/definitions, we do not support the builtin
   // attribute. We do not check this in VerifyFunctionAttrs since that is
   // checking for Attributes that can/can not ever be on functions.
   Assert(!Attrs.hasFnAttr(Attribute::Builtin),
          "Attribute 'builtin' can only be applied to a callsite.", &F);
 
   Assert(!Attrs.hasAttrSomewhere(Attribute::ElementType),
for (unsigned i = 0, e = CBI.getNumOperands(); i != e; ++i) {
     SmallPtrSet<BasicBlock *, 4> ArgBBs;
     for (Value *V : CBI.args())
       if (auto *BA = dyn_cast<BlockAddress>(V))
         ArgBBs.insert(BA->getBasicBlock());
     for (BasicBlock *BB : CBI.getIndirectDests())
       Assert(ArgBBs.count(BB), "Indirect label missing from arglist.", &CBI);
   }
 
+  verifyInlineAsmCall(CBI);
   visitTerminator(CBI);
 }
 
 void Verifier::visitSelectInst(SelectInst &SI) {
   Assert(!SelectInst::areInvalidOperands(SI.getOperand(0), SI.getOperand(1),
                                          SI.getOperand(2)),
          "Invalid operands for select instruction!", &SI);
 
void Verifier::visitCallBase(CallBase &Call) {
   if (Attrs.hasFnAttr(Attribute::Preallocated)) {
     Assert(Call.getCalledFunction()->getIntrinsicID() ==
                Intrinsic::call_preallocated_arg,
            "preallocated as a call site attribute can only be on "
            "llvm.call.preallocated.arg");
   }
 
   // Verify call attributes.
-  verifyFunctionAttrs(FTy, Attrs, &Call, IsIntrinsic);
+  verifyFunctionAttrs(FTy, Attrs, &Call, IsIntrinsic, Call.isInlineAsm());
 
   // Conservatively check the inalloca argument.
   // We have a bug if we can find that there is an underlying alloca without
   // inalloca.
   if (Call.hasInAllocaArgument()) {
     Value *InAllocaArg = Call.getArgOperand(FTy->getNumParams() - 1);
     if (auto AI = dyn_cast<AllocaInst>(InAllocaArg->stripInBoundsOffsets()))
       Assert(AI->isUsedWithInAlloca(),
void Verifier::visitCallBase(CallBase &Call) {
   // do so causes assertion failures when the inliner sets up inline scope info.
   if (Call.getFunction()->getSubprogram() && Call.getCalledFunction() &&
       Call.getCalledFunction()->getSubprogram())
     AssertDI(Call.getDebugLoc(),
              "inlinable function call in a function with "
              "debug info must have a !dbg location",
              Call);
 
+  if (Call.isInlineAsm())
+    verifyInlineAsmCall(Call);
+
   visitInstruction(Call);
 }
 
 void Verifier::verifyTailCCMustTailAttrs(const AttrBuilder &Attrs,
                                          StringRef Context) {
   Assert(!Attrs.contains(Attribute::InAlloca),
          Twine("inalloca attribute not allowed in ") + Context);
   Assert(!Attrs.contains(Attribute::InReg),

llvm/test/Verifier/elementtype.ll

 ; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
 
 declare void @some_function(i32*)
 
 ; CHECK: Attribute 'elementtype(i32)' applied to incompatible type!
 define void @type_mismatch1() {
   call i32* @llvm.preserve.array.access.index.p0i32.p0i32(i32* null, i32 elementtype(i32) 0, i32 0)
   ret void
 }
 
 ; CHECK: Attribute 'elementtype' type does not match parameter!
 define void @type_mismatch2() {
   call i32* @llvm.preserve.array.access.index.p0i32.p0i32(i32* elementtype(i64) null, i32 0, i32 0)
   ret void
 }
 
-; CHECK: Attribute 'elementtype' can only be applied to intrinsics.
+; CHECK: Attribute 'elementtype' can only be applied to intrinsics and inline asm.
 define void @not_intrinsic() {
   call void @some_function(i32* elementtype(i32) null)
   ret void
 }
 
 ; CHECK: Attribute 'elementtype' can only be applied to a callsite.
 define void @llvm.not_call(i32* elementtype(i32)) {
   ret void

llvm/test/Verifier/inline-asm-indirect-operand.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+
+define void @okay(i32* %p, i32 %x) {
+  call void asm "addl $1, $0", "=*rm,r"(i32* elementtype(i32) %p, i32 %x)
+  ret void
+}
+
+; CHECK: Attribute 'elementtype' type does not match parameter!
+; CHECK-NEXT: call void asm "addl $1, $0", "=*rm,r"(i32* elementtype(i64) %p, i32 %x)
+define void @wrong_element_type(i32* %p, i32 %x) {
+  call void asm "addl $1, $0", "=*rm,r"(i32* elementtype(i64) %p, i32 %x)
+  ret void
+}
+
+; CHECK: Operand for indirect constraint must have pointer type
+; CHECK-NEXT: call void asm "addl $1, $0", "=*rm,r"(i32 %p, i32 %x)
+define void @not_pointer_arg(i32 %p, i32 %x) {
+  call void asm "addl $1, $0", "=*rm,r"(i32 %p, i32 %x)
+  ret void
+}
+
+; CHECK: Elementtype attribute can only be applied for indirect constraints
+; CHECK-NEXT: call void asm "addl $1, $0", "=*rm,r"(i32* %p, i32* elementtype(i32) %x)
+define void @not_indirect(i32* %p, i32* %x) {
+  call void asm "addl $1, $0", "=*rm,r"(i32* %p, i32* elementtype(i32) %x)
+  ret void
+}
+
+; CHECK: Operand for indirect constraint must have pointer type
+; CHECK-NEXT: invoke void asm "addl $1, $0", "=*rm,r"(i32 %p, i32 %x)
+define void @not_pointer_arg_invoke(i32 %p, i32 %x) personality i8* null {
+  invoke void asm "addl $1, $0", "=*rm,r"(i32 %p, i32 %x)
+      to label %cont unwind label %lpad
+
+lpad:
+  %lp = landingpad i32
+      cleanup
+  ret void
+
+cont:
+  ret void
+}
+
+; CHECK: Operand for indirect constraint must have pointer type
+; CHECK-NEXT: callbr void asm "addl $1, $0", "=*rm,r"(i32 %p, i32 %x)
+define void @not_pointer_arg_callbr(i32 %p, i32 %x) {
+  callbr void asm "addl $1, $0", "=*rm,r"(i32 %p, i32 %x)
+      to label %cont []
+
+cont:
+  ret void
+}