Ignore cleared debug instructions

[jarcherNV]

Debug instructions can be cleared during debug or non-semantic stripping passes. Check for this before attempting to access their operands.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[jarcherNV]

I've noticed a similar issue has been filed for this bug previously, which I believe should be fixed by this PR: #5912

[s-perron]

How hard would it be to clear the cached value when the instruction was deleted? It seems like this is a special case that could cause problems in other places too.

[jarcherNV]

I'm not familiar enough with the code to be able to say how easy that would be. Do you know where the cached value is set/cleared?

[jarcherNV]

For reference, I believe the clearing is done in strip_debug_info_pass.cpp at:

// clear OpLine information
context()->module()->ForEachInst([&modified](Instruction* inst) {
modified |= !inst->dbg_line_insts().empty();
inst->dbg_line_insts().clear();
});

[s-perron]

I'm not familiar enough with the code to be able to say how easy that would be. Do you know where the cached value is set/cleared?

I would expect in a debug build for this check to fail after the pass that removes the instruciton because the feature manager is no longer correct:

https://github.com/s-perron/SPIRV-Tools/blob/91581ceff76db2ee1eea5698c40548bba715ed42/source/opt/feature_manager.cpp#L107

I would simply call IRContext::ResetFeatureManager() in that pass. This is a guess without looking at any specific examples.

[s-perron]

A little more context. After every pass, there is an expectations that all analysies that the context thinks are valid, actually are. This is checked by calling

SPIRV-Tools/source/opt/ir_context.cpp

Lines 374 to 378 in 0102146

	bool IRContext::IsConsistent() {
	#ifndef SPIRV_CHECK_CONTEXT
	return true;
	#else
	if (AreAnalysesValid(kAnalysisDefUse)) {

Based on what is happening, I would expect IsConsistent to return false because the feature manager is not correct after the instruction has been cleared. Can you try building with this check enabled? In Cmake, you need to have a debug build and the SPIRV_CHECK_CONTEXT option turned on (which is the default). Then I would expect a pass to fail this assert:

SPIRV-Tools/source/opt/pass.cpp

Lines 43 to 45 in 0102146

	if (!(status == Status::Failure || ctx->IsConsistent()))
	assert(false && "An analysis in the context is out of date.");
	return status;

[jarcherNV]

Ahh thanks, I was not aware of that. I'll give that a try and add the call to IRContext::ResetFeatureManager() if needed.

[jarcherNV]

Sorry I have not had a chance to come back to this one as we ended up going in a slightly different direction that no longer needed this PR. I'm not sure if I'll have time to finish it. Should I close this so that it doesn't clog up the backlog of open PRs?

[s-perron]

Thanks for letting us know. Can you open an issue for this, and then we can close the PR?

[alister-chowdhury]

FWIW:

How hard would it be to clear the cached value when the instruction was deleted? It seems like this is a special case that could cause problems in other places too.

That is what I'm attempting here: #6416