From 9c22a62ae4231fe97b93fdc2e3fae413bca3d503 Mon Sep 17 00:00:00 2001 From: Sameer Sahasrabuddhe Date: Wed, 13 Sep 2023 16:36:36 +0530 Subject: [PATCH 1/3] [LLVM] convergence verifier should visit all instructions The entry and loop intrinsics for convergence control cannot be preceded by convergent operations in their respective basic blocks. To check that, the verifier needs to reset its state at the start of the block. This was missed in the previous commit fa6dd7a24af2b02f236ec3b980d9407e86c2c4aa. --- llvm/lib/IR/Verifier.cpp | 4 ++-- llvm/test/Verifier/convergencectrl-invalid.ll | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp index c0f30a62b8bcc..9ed66e6af68fb 100644 --- a/llvm/lib/IR/Verifier.cpp +++ b/llvm/lib/IR/Verifier.cpp @@ -3576,8 +3576,6 @@ void Verifier::visitCallBase(CallBase &Call) { if (Call.isInlineAsm()) verifyInlineAsmCall(Call); - CV.visit(Call); - visitInstruction(Call); } @@ -4805,6 +4803,8 @@ void Verifier::visitInstruction(Instruction &I) { BasicBlock *BB = I.getParent(); Check(BB, "Instruction not embedded in basic block!", &I); + CV.visit(I); + if (!isa(I)) { // Check that non-phi nodes are not self referential for (User *U : I.users()) { Check(U != (User *)&I || !DT.isReachableFromEntry(BB), diff --git a/llvm/test/Verifier/convergencectrl-invalid.ll b/llvm/test/Verifier/convergencectrl-invalid.ll index 2f7b311973d7e..e1fffcd1c6033 100644 --- a/llvm/test/Verifier/convergencectrl-invalid.ll +++ b/llvm/test/Verifier/convergencectrl-invalid.ll @@ -126,13 +126,18 @@ define void @entry_in_convergent(i32 %x, i32 %y) { } ; CHECK: Loop intrinsic cannot be preceded by a convergent operation in the same basic block. -; CHECK: %t60_tok3 +; CHECK-NEXT: %h1 +; CHECK-SAME: %t60_tok3 define void @loop_at_start(i32 %x, i32 %y) convergent { A: %t60_tok3 = call token @llvm.experimental.convergence.entry() br label %B B: %z = add i32 %x, %y + ; This is not an error + %h2 = call token @llvm.experimental.convergence.loop() [ "convergencectrl"(token %t60_tok3) ] + br label %C +C: call void @f() %h1 = call token @llvm.experimental.convergence.loop() [ "convergencectrl"(token %t60_tok3) ] ret void From 0257ab3cda22479f0d0645f11fddcd00ca773f8e Mon Sep 17 00:00:00 2001 From: Sameer Sahasrabuddhe Date: Thu, 14 Sep 2023 12:21:08 +0530 Subject: [PATCH 2/3] visit each basic block once to reset the first convergent op --- llvm/include/llvm/ADT/GenericConvergenceVerifier.h | 1 + llvm/include/llvm/IR/GenericConvergenceVerifierImpl.h | 10 +++++----- llvm/lib/IR/Verifier.cpp | 5 +++-- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/llvm/include/llvm/ADT/GenericConvergenceVerifier.h b/llvm/include/llvm/ADT/GenericConvergenceVerifier.h index 71b7b41ef9666..0810a07013229 100644 --- a/llvm/include/llvm/ADT/GenericConvergenceVerifier.h +++ b/llvm/include/llvm/ADT/GenericConvergenceVerifier.h @@ -40,6 +40,7 @@ template class GenericConvergenceVerifier { } void clear(); + void visit(const BlockT &BB); void visit(const InstructionT &I); void verify(const DominatorTreeT &DT); diff --git a/llvm/include/llvm/IR/GenericConvergenceVerifierImpl.h b/llvm/include/llvm/IR/GenericConvergenceVerifierImpl.h index c57f828cb1de7..2ba81015cb7b6 100644 --- a/llvm/include/llvm/IR/GenericConvergenceVerifierImpl.h +++ b/llvm/include/llvm/IR/GenericConvergenceVerifierImpl.h @@ -67,17 +67,17 @@ template void GenericConvergenceVerifier::clear() { ConvergenceKind = NoConvergence; } +template +void GenericConvergenceVerifier::visit(const BlockT &BB) { + SeenFirstConvOp = false; +} + template void GenericConvergenceVerifier::visit(const InstructionT &I) { auto ID = ContextT::getIntrinsicID(I); auto *TokenDef = findAndCheckConvergenceTokenUsed(I); bool IsCtrlIntrinsic = true; - // If this is the first instruction in the block, then we have not seen a - // convergent op yet. - if (!I.getPrevNode()) - SeenFirstConvOp = false; - switch (ID) { case Intrinsic::experimental_convergence_entry: Check(isInsideConvergentFunction(I), diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp index 9ed66e6af68fb..406cb46f7abe9 100644 --- a/llvm/lib/IR/Verifier.cpp +++ b/llvm/lib/IR/Verifier.cpp @@ -2875,6 +2875,7 @@ void Verifier::visitFunction(const Function &F) { // void Verifier::visitBasicBlock(BasicBlock &BB) { InstsInThisBlock.clear(); + CV.visit(BB); // Ensure that basic blocks have terminators! Check(BB.getTerminator(), "Basic Block does not have terminator!", &BB); @@ -3576,6 +3577,8 @@ void Verifier::visitCallBase(CallBase &Call) { if (Call.isInlineAsm()) verifyInlineAsmCall(Call); + CV.visit(Call); + visitInstruction(Call); } @@ -4803,8 +4806,6 @@ void Verifier::visitInstruction(Instruction &I) { BasicBlock *BB = I.getParent(); Check(BB, "Instruction not embedded in basic block!", &I); - CV.visit(I); - if (!isa(I)) { // Check that non-phi nodes are not self referential for (User *U : I.users()) { Check(U != (User *)&I || !DT.isReachableFromEntry(BB), From ed1df5e484172f3a2f2ecc953fe134649c19fd28 Mon Sep 17 00:00:00 2001 From: Sameer Sahasrabuddhe Date: Thu, 14 Sep 2023 14:06:13 +0530 Subject: [PATCH 3/3] rename CV to ConvergenceVerifyHelper, similar to other helpers --- llvm/lib/IR/Verifier.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp index 406cb46f7abe9..7cbd7401c92c6 100644 --- a/llvm/lib/IR/Verifier.cpp +++ b/llvm/lib/IR/Verifier.cpp @@ -363,7 +363,7 @@ class Verifier : public InstVisitor, VerifierSupport { SmallVector DebugFnArgs; TBAAVerifier TBAAVerifyHelper; - ConvergenceVerifier CV; + ConvergenceVerifier ConvergenceVerifyHelper; SmallVector NoAliasScopeDecls; @@ -408,15 +408,15 @@ class Verifier : public InstVisitor, VerifierSupport { auto FailureCB = [this](const Twine &Message) { this->CheckFailed(Message); }; - CV.initialize(OS, FailureCB, F); + ConvergenceVerifyHelper.initialize(OS, FailureCB, F); Broken = false; // FIXME: We strip const here because the inst visitor strips const. visit(const_cast(F)); verifySiblingFuncletUnwinds(); - if (CV.sawTokens()) - CV.verify(DT); + if (ConvergenceVerifyHelper.sawTokens()) + ConvergenceVerifyHelper.verify(DT); InstsInThisBlock.clear(); DebugFnArgs.clear(); @@ -2875,7 +2875,7 @@ void Verifier::visitFunction(const Function &F) { // void Verifier::visitBasicBlock(BasicBlock &BB) { InstsInThisBlock.clear(); - CV.visit(BB); + ConvergenceVerifyHelper.visit(BB); // Ensure that basic blocks have terminators! Check(BB.getTerminator(), "Basic Block does not have terminator!", &BB); @@ -3577,7 +3577,7 @@ void Verifier::visitCallBase(CallBase &Call) { if (Call.isInlineAsm()) verifyInlineAsmCall(Call); - CV.visit(Call); + ConvergenceVerifyHelper.visit(Call); visitInstruction(Call); }