Skip to content

[LLVM] convergence verifier should visit all instructions #66200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 20, 2023
Merged

[LLVM] convergence verifier should visit all instructions #66200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9c22a62
[LLVM] convergence verifier should visit all instructions
ssahasra Sep 13, 2023
0257ab3
visit each basic block once to reset the first convergent op
ssahasra Sep 14, 2023
ed1df5e
rename CV to ConvergenceVerifyHelper, similar to other helpers
ssahasra Sep 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions llvm/include/llvm/ADT/GenericConvergenceVerifier.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ template <typename ContextT> class GenericConvergenceVerifier {
}

void clear();
void visit(const BlockT &BB);
void visit(const InstructionT &I);
void verify(const DominatorTreeT &DT);

Expand Down
10 changes: 5 additions & 5 deletions llvm/include/llvm/IR/GenericConvergenceVerifierImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,17 @@ template <class ContextT> void GenericConvergenceVerifier<ContextT>::clear() {
ConvergenceKind = NoConvergence;
}

template <class ContextT>
void GenericConvergenceVerifier<ContextT>::visit(const BlockT &BB) {
SeenFirstConvOp = false;
}

template <class ContextT>
void GenericConvergenceVerifier<ContextT>::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),
Expand Down
11 changes: 6 additions & 5 deletions llvm/lib/IR/Verifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
SmallVector<const DILocalVariable *, 16> DebugFnArgs;

TBAAVerifier TBAAVerifyHelper;
ConvergenceVerifier CV;
ConvergenceVerifier ConvergenceVerifyHelper;

SmallVector<IntrinsicInst *, 4> NoAliasScopeDecls;

Expand Down Expand Up @@ -408,15 +408,15 @@ class Verifier : public InstVisitor<Verifier>, 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<Function &>(F));
verifySiblingFuncletUnwinds();

if (CV.sawTokens())
CV.verify(DT);
if (ConvergenceVerifyHelper.sawTokens())
ConvergenceVerifyHelper.verify(DT);

InstsInThisBlock.clear();
DebugFnArgs.clear();
Expand Down Expand Up @@ -2875,6 +2875,7 @@ void Verifier::visitFunction(const Function &F) {
//
void Verifier::visitBasicBlock(BasicBlock &BB) {
InstsInThisBlock.clear();
ConvergenceVerifyHelper.visit(BB);

// Ensure that basic blocks have terminators!
Check(BB.getTerminator(), "Basic Block does not have terminator!", &BB);
Expand Down Expand Up @@ -3576,7 +3577,7 @@ void Verifier::visitCallBase(CallBase &Call) {
if (Call.isInlineAsm())
verifyInlineAsmCall(Call);

CV.visit(Call);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If only calls can be convergent, I'm not sure I see how this is different. I don't really expect a visit instruction for the verifier to be side effecting. Can you have a separate enter block visitor?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ConvergenceVerifier may also keep record of the last-call-instruction's parent. Then it is easy to detect entering a new block inside ConvergenceVerifier::visit(). I have no opinion which is the best way to way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this by adding a visitBlock() method to the convergence verifier.

ConvergenceVerifyHelper.visit(Call);

visitInstruction(Call);
}
Expand Down
7 changes: 6 additions & 1 deletion llvm/test/Verifier/convergencectrl-invalid.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) ]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make that explicit with a CHECK-NOT: %h2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what the "CHECK-NEXT: %h1" does. It ensures that the error message is for %h1 and not %h2. There is not enough context in the stream of error messages to put a CHECK-NOT here.

br label %C
C:
call void @f()
%h1 = call token @llvm.experimental.convergence.loop() [ "convergencectrl"(token %t60_tok3) ]
ret void
Expand Down