[Verifier] Verify scoped noalias metadata

[nikic]

Summary

Verify that !noalias, !alias.scope and llvm.experimental.noalias.scope arguments have the format specified in https://llvm.org/docs/LangRef.html#noalias-and-alias-scope-metadata. I've fixed up a lot of broken metadata used by tests in advance. Especially using a scope instead of the expected scope list is a commonly made mistake.

[jeroen.dobbelaere]

LGTM

llvm/lib/IR/Verifier.cpp

4350

Did you run clang-format ?

[nikic]

@Meinersbur I partially fixed the alias metadata generated by polly in https://github.com/llvm/llvm-project/commit/53720f74e4e32fe11a1688282f7d09dc1828b83a. However, another fix for the "second level" metadata is needed. This code is doing something very odd in https://github.com/llvm/llvm-project/blob/53720f74e4e32fe11a1688282f7d09dc1828b83a/polly/lib/CodeGen/IRBuilder.cpp#L207 where it apparently creates an alias scope with another alias scope used as a domain. It wasn't obvious to me what the right way to adjust this code is.

[Meinersbur]

@nikic Thanks for fixing Polly. Regarding the "second level" metadata, it represents another source known non-aliasing from internal "Inter iteration alias-free" annotations which were added as part of a GSoC. I am going to have a look on what it is trying to achieve.

[Meinersbur]

@nikic I ended up removing the second level metadata in https://github.com/llvm/llvm-project/commit/cad9f98a2ad98fecf663e9ce39502b8e43676fc9. Thanks again for fixing the fixing the array-based noalias metadata.

[hgreving]

Seems like that this patch now contradicts the methods MDBuilder.h:createAnonymousAliasScopeDomain and createAnonymousAliasScope. If you actually give it a string, it won't verify. I wonder if you should amend this changing the methods as well?

[nikic]

In D110026#3012747, @hgreving wrote:

Seems like that this patch now contradicts the methods MDBuilder.h:createAnonymousAliasScopeDomain and createAnonymousAliasScope. If you actually give it a string, it won't verify. I wonder if you should amend this changing the methods as well?

These methods are intended to be supported and are used in-tree and do verify there. Could you provide an example that would fail? Please keep in mind that scoped alias metadata always deals in scope lists, while createAnonymousAliasScope() only creates an isolated scope. You cannot pass the result of createAnonymousAliasScope() directly to !noalias, !alias.scope, etc.

[hgreving]

In D110026#3012827, @nikic wrote:

In D110026#3012747, @hgreving wrote:

Seems like that this patch now contradicts the methods MDBuilder.h:createAnonymousAliasScopeDomain and createAnonymousAliasScope. If you actually give it a string, it won't verify. I wonder if you should amend this changing the methods as well?

These methods are intended to be supported and are used in-tree and do verify there. Could you provide an example that would fail? Please keep in mind that scoped alias metadata always deals in scope lists, while createAnonymousAliasScope() only creates an isolated scope. You cannot pass the result of createAnonymousAliasScope() directly to !noalias, !alias.scope, etc.

Yes. But MDB.createAnonymousAliasScopeDomain("mystring") noew creates e.g. the domain !33 = distinct !{!33, !"mystring"}, which leads to the verifier error "second scope operand must be MDNode". This seems inconsistent. Same happens for createAnonymousAliasScope().

[hgreving]

In D110026#3012843, @hgreving wrote:

In D110026#3012827, @nikic wrote:

In D110026#3012747, @hgreving wrote:

Seems like that this patch now contradicts the methods MDBuilder.h:createAnonymousAliasScopeDomain and createAnonymousAliasScope. If you actually give it a string, it won't verify. I wonder if you should amend this changing the methods as well?

These methods are intended to be supported and are used in-tree and do verify there. Could you provide an example that would fail? Please keep in mind that scoped alias metadata always deals in scope lists, while createAnonymousAliasScope() only creates an isolated scope. You cannot pass the result of createAnonymousAliasScope() directly to !noalias, !alias.scope, etc.

Yes. But MDB.createAnonymousAliasScopeDomain("mystring") noew creates e.g. the domain !33 = distinct !{!33, !"mystring"}, which leads to the verifier error "second scope operand must be MDNode". This seems inconsistent. Same happens for createAnonymousAliasScope().

I take it back. My confusion came from the fact that we were not creating lists as you pointed out above! Thanks for the tip, I think the methods are fine.

llvm/lib/IR/Verifier.cpp

void visitModuleFlag(const MDNode *Op,
                        SmallVectorImpl<const MDNode *> &Requirements);
   void visitModuleFlagCGProfileEntry(const MDOperand &MDO);
   void visitFunction(const Function &F);
   void visitBasicBlock(BasicBlock &BB);
   void visitRangeMetadata(Instruction &I, MDNode *Range, Type *Ty);
   void visitDereferenceableMetadata(Instruction &I, MDNode *MD);
   void visitProfMetadata(Instruction &I, MDNode *MD);
   void visitAnnotationMetadata(MDNode *Annotation);
+  void visitAliasScopeMetadata(const MDNode *MD);
+  void visitAliasScopeListMetadata(const MDNode *MD);
 
   template <class Ty> bool isValidMetadataArray(const MDTuple &N);
 #define HANDLE_SPECIALIZED_MDNODE_LEAF(CLASS) void visit##CLASS(const CLASS &N);
 #include "llvm/IR/Metadata.def"
   void visitDIScope(const DIScope &N);
   void visitDIVariable(const DIVariable &N);
   void visitDILexicalBlockBase(const DILexicalBlockBase &N);
   void visitDITemplateParameter(const DITemplateParameter &N);

 void Verifier::visitAnnotationMetadata(MDNode *Annotation) {
   Assert(isa<MDTuple>(Annotation), "annotation must be a tuple");
   Assert(Annotation->getNumOperands() >= 1,
          "annotation must have at least one operand");
   for (const MDOperand &Op : Annotation->operands())
     Assert(isa<MDString>(Op.get()), "operands must be strings");
 }
 
+void Verifier::visitAliasScopeMetadata(const MDNode *MD) {
+  unsigned NumOps = MD->getNumOperands();
+  Assert(NumOps >= 2 && NumOps <= 3, "scope must have two or three operands",
+         MD);
+  Assert(MD->getOperand(0) == MD || isa<MDString>(MD->getOperand(0)),
+         "first scope operand must be self-referential or string", MD);
+  if (NumOps == 3)
+    Assert(isa<MDString>(MD->getOperand(2)),
+           "third scope operand must be string (if used)", MD);
+
+  MDNode *Domain = dyn_cast<MDNode>(MD->getOperand(1));
+  Assert(Domain != nullptr, "second scope operand must be MDNode", MD);
+
+  unsigned NumDomainOps = Domain->getNumOperands();
+  Assert(NumDomainOps >= 1 && NumDomainOps <= 2,
+         "domain must have one or two operands", Domain);
+  Assert(Domain->getOperand(0) == Domain ||
+             isa<MDString>(Domain->getOperand(0)),
+         "first domain operand must be self-referential or string", Domain);
+  if (NumDomainOps == 2)
+    Assert(isa<MDString>(Domain->getOperand(1)),
+           "second domain operand must be string (if used)", Domain);
+}
+
+void Verifier::visitAliasScopeListMetadata(const MDNode *MD) {
+  for (const MDOperand &Op : MD->operands()) {
+    const MDNode *OpMD = dyn_cast<MDNode>(Op);
+    Assert(OpMD != nullptr, "scope list must consist of MDNodes", MD);
+    visitAliasScopeMetadata(OpMD);
+  }
+}
+
 /// verifyInstruction - Verify that an instruction is well formed.
 ///
 void Verifier::visitInstruction(Instruction &I) {
   BasicBlock *BB = I.getParent();
   Assert(BB, "Instruction not embedded in basic block!", &I);
 
   if (!isa<PHINode>(I)) {   // Check that non-phi nodes are not self referential
     for (User *U : I.users()) {
if (MDNode *MD = I.getMetadata(LLVMContext::MD_dereferenceable))
     visitDereferenceableMetadata(I, MD);
 
   if (MDNode *MD = I.getMetadata(LLVMContext::MD_dereferenceable_or_null))
     visitDereferenceableMetadata(I, MD);
 
   if (MDNode *TBAA = I.getMetadata(LLVMContext::MD_tbaa))
     TBAAVerifyHelper.visitTBAAMetadata(I, TBAA);
 
+  if (MDNode *MD = I.getMetadata(LLVMContext::MD_noalias))
+    visitAliasScopeListMetadata(MD);
+  if (MDNode *MD = I.getMetadata(LLVMContext::MD_alias_scope))
+    visitAliasScopeListMetadata(MD);
+
   if (MDNode *AlignMD = I.getMetadata(LLVMContext::MD_align)) {
     Assert(I.getType()->isPointerTy(), "align applies only to pointer types",
            &I);
     Assert(isa<LoadInst>(I), "align applies only to load instructions, "
            "use attributes for calls or invokes", &I);
     Assert(AlignMD->getNumOperands() == 1, "align takes one operand!", &I);
     ConstantInt *CI = mdconst::dyn_extract<ConstantInt>(AlignMD->getOperand(0));
     Assert(CI && CI->getType()->isIntegerTy(64),
Assert(ScopeListMV != nullptr,
            "argument",
            II);
 
     const auto *ScopeListMD = dyn_cast<MDNode>(ScopeListMV->getMetadata());
     Assert(ScopeListMD != nullptr, "!id.scope.list must point to an MDNode",
            II);
     Assert(ScopeListMD->getNumOperands() == 1,
            "!id.scope.list must point to a list with a single scope", II);
+    visitAliasScopeListMetadata(ScopeListMD);
   }
 
   // Only check the domination rule when requested. Once all passes have been
   // adapted this option can go away.
   if (!VerifyNoAliasScopeDomination)
     return;
 
   // Now sort the intrinsics based on the scope MDNode so that declarations of

llvm/test/Verifier/alias-scope-metadata.ll

+; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
+
+define void @test(i8* %p) {
+  load i8, i8* %p, !noalias !0
+  load i8, i8* %p, !noalias !1
+  load i8, i8* %p, !noalias !3
+  load i8, i8* %p, !noalias !5
+  load i8, i8* %p, !noalias !7
+  load i8, i8* %p, !noalias !9
+  load i8, i8* %p, !noalias !11
+  load i8, i8* %p, !noalias !14
+  load i8, i8* %p, !alias.scope !17
+  call void @llvm.experimental.noalias.scope.decl(metadata !20)
+  ret void
+}
+
+declare void @llvm.experimental.noalias.scope.decl(metadata)
+
+; CHECK: scope list must consist of MDNodes
+!0 = !{!"str"}
+
+; CHECK: scope must have two or three operands
+!1 = !{!2}
+!2 = !{!2}
+
+; CHECK: scope must have two or three operands
+!3 = !{!4}
+!4 = !{!4, !5, !6, !7}
+
+; CHECK: first scope operand must be self-referential or string
+!5 = !{!6}
+!6 = !{!7, !8}
+
+; CHECK: third scope operand must be string (if used)
+!7 = !{!8}
+!8 = !{!8, !9, !10}
+
+; CHECK: second scope operand must be MDNode
+!9 = !{!10}
+!10 = !{!10, !"str"}
+
+; CHECK: domain must have one or two operands
+!11 = !{!12}
+!12 = !{!12, !13}
+!13 = !{}
+
+; CHECK: domain must have one or two operands
+!14 = !{!15}
+!15 = !{!15, !16}
+!16 = !{!17, !18, !19}
+
+; CHECK: first domain operand must be self-referential or string
+!17 = !{!18}
+!18 = !{!18, !19}
+!19 = !{!20}
+
+; CHECK: second domain operand must be string (if used)
+!20 = !{!21}
+!21 = !{!21, !22}
+!22 = !{!22, !23}
+!23 = !{}