[Convergence] allow non-convergent ops before entry and loop intrinsics #65939
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The only real requirement is that entry and loop intrinsics should not be preceded by convergent operations in the same basic block. They do not need to be the first in the block. Relaxing the constraint on the entry and loop intrinsics avoids having to make changes in the construction of LLVM IR, such as getFirstInsertionPt(). It also avoids added complexity in the lowering to Machine IR, where COPY instructions may be added to the start of the basic block.
nhaehnle
approved these changes
Sep 11, 2023
ruiling
reviewed
Sep 11, 2023
| @@ -63,6 +63,8 @@ template <typename ContextT> class GenericConvergenceVerifier { | |||
| // and not the token values. | |||
| DenseMap<const InstructionT *, const InstructionT *> Tokens; | |||
|
|
|||
| bool SeenFirstConvOp = false; | |||
There was a problem hiding this comment.
The value will not be reset across basic block, right?
There was a problem hiding this comment.
It gets reset at the start of every block in the visit method.
There was a problem hiding this comment.
I think not? The visit() method you pointed out is only called in Verifier::visitCallBase(). So it cannot reset the flag in a new block.
There was a problem hiding this comment.
Ouch! You are right. I was paying too much attention on the MachineConvergenceVerifier (not checked in yet), and didn't see how the ConvergenceVerifier traverses the function. I have a fix in my branch, will push it out real soo now. Thanks for catching it!
ZijunZhaoCCK
pushed a commit
to ZijunZhaoCCK/llvm-project
that referenced
this pull request
Sep 19, 2023
…cs (llvm#65939) The only real requirement is that entry and loop intrinsics should not be preceded by convergent operations in the same basic block. They do not need to be the first in the block. Relaxing the constraint on the entry and loop intrinsics avoids having to make changes in the construction of LLVM IR, such as getFirstInsertionPt(). It also avoids added complexity in the lowering to Machine IR, where COPY instructions may be added to the start of the basic block.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The only real requirement is that entry and loop intrinsics should not be preceded by convergent operations in the same basic block. They do not need to be the first in the block.
Relaxing the constraint on the entry and loop intrinsics avoids having to make changes in the construction of LLVM IR, such as getFirstInsertionPt(). It also avoids added complexity in the lowering to Machine IR, where COPY instructions may be added to the start of the basic block.