PIX: Don't seek beyond terminator instructions (value-to-declare pass)#3855
Merged
jeffnn merged 1 commit intomicrosoft:masterfrom Jun 30, 2021
Merged
Conversation
|
✅ Build DirectXShaderCompiler 1.0.324 completed (commit 89a485260f by @) |
pow2clk
approved these changes
Jun 30, 2021
Collaborator
pow2clk
left a comment
pow2clk
left a comment
There was a problem hiding this comment.
I think one of the checks is redundant, but perhaps that'll be resolved when you have more time to clean this up definitively
| } while (instruction != nullptr && llvm::isa<llvm::PHINode>(instruction)); | ||
| } while (instruction != nullptr && | ||
| llvm::isa<llvm::PHINode>(instruction) && | ||
| !llvm::isa<TerminatorInst>(instruction)); |
Collaborator
There was a problem hiding this comment.
I don't think this is necessary as a PHI Node can't be a Terminator, it's harmless though.
jeffnn
added a commit
to jeffnn/DirectXShaderCompiler
that referenced
this pull request
Jun 30, 2021
microsoft#3855) Background: this pass is trying to find all dbg.value and replace them with dbg.declare. In the code being changed, the pass is trying to seek a valid location at which to insert the dbg.declare. It has to come after the value to which it applies (which isn't true of the dbg.value). So there's this little loop trying to move forward to find the right instruction before which to insert new stuff. I was expecting getNextNode to return null when there is no next node. When called on a terminator, it actually returns a non-null but malformed instruction pointer. So we have to explicitly check for terminators in this loop. This really short basic block tripped up the pass: ; <label>:274 ; preds = %.lr.ph55 %RawBufferLoad = call %dx.types.ResRet.i32 @dx.op.rawBufferLoad.i32(i32 139, %dx.types.Handle %lightBuffer_texture_structbuf, i32 %lightIndex.0, i32 28, i8 1, i32 4), !dbg !384 %275 = extractvalue %dx.types.ResRet.i32 %RawBufferLoad, 0, !dbg !384 switch i32 %275, label %288 [ i32 0, label %276 i32 1, label %280 i32 2, label %284 ], !dbg !397 I think the pass could be smarter about seeking the right insertion point for the dbg.declare. It's currently assuming that the dbg.value always succeeds the value to which it refers, but as in this case, that's not always true. But that's a project for another day. (cherry picked from commit 650de80)
jeffnn
added a commit
that referenced
this pull request
Jun 30, 2021
#3855) (#3856) Background: this pass is trying to find all dbg.value and replace them with dbg.declare. In the code being changed, the pass is trying to seek a valid location at which to insert the dbg.declare. It has to come after the value to which it applies (which isn't true of the dbg.value). So there's this little loop trying to move forward to find the right instruction before which to insert new stuff. I was expecting getNextNode to return null when there is no next node. When called on a terminator, it actually returns a non-null but malformed instruction pointer. So we have to explicitly check for terminators in this loop. This really short basic block tripped up the pass: ; <label>:274 ; preds = %.lr.ph55 %RawBufferLoad = call %dx.types.ResRet.i32 @dx.op.rawBufferLoad.i32(i32 139, %dx.types.Handle %lightBuffer_texture_structbuf, i32 %lightIndex.0, i32 28, i8 1, i32 4), !dbg !384 %275 = extractvalue %dx.types.ResRet.i32 %RawBufferLoad, 0, !dbg !384 switch i32 %275, label %288 [ i32 0, label %276 i32 1, label %280 i32 2, label %284 ], !dbg !397 I think the pass could be smarter about seeking the right insertion point for the dbg.declare. It's currently assuming that the dbg.value always succeeds the value to which it refers, but as in this case, that's not always true. But that's a project for another day. (cherry picked from commit 650de80)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Background: this pass is trying to find all dbg.value and replace them with dbg.declare. In the code being changed, the pass is trying to seek a valid location at which to insert the dbg.declare. It has to come after the value to which it applies (which isn't true of the dbg.value). So there's this little loop trying to move forward to find the right instruction before which to insert new stuff.
I was expecting getNextNode to return null when there is no next node. When called on a terminator, it actually returns a non-null but malformed instruction pointer. So we have to explicitly check for terminators in this loop.
This really short basic block tripped up the pass:
I think the pass could be smarter about seeking the right insertion point for the dbg.declare. It's currently assuming that the dbg.value always succeeds the value to which it refers, but as in this case, that's not always true. But that's a project for another day.