Assert in optimization pass with new SPIRV-Tools

[pow2clk]

A recent SPIRV-Tools change altered which Analyses were preserved for the compact IDs pass:

commit a1f9e1342e77e47251e2f001d95f8af8a523745a
Author: Steven Perron stevenperron@google.com
Date: Fri Jun 1 13:04:46 2018 -0400
Preserve inst-to-block and def-use in passes.

Seems it's related to: KhronosGroup/SPIRV-Tools#1593

The result of this for us is that instead of skipping that optimization pass entirely because there were no Analyses to do, it proceeds and tries to extract information from a block that doesn't seem to exist in the context, but it existed in the module.

I have no reason to believe that the change in question is the actual cause, but it is the inflection point where the problem began. So if it is not to blame, it exposed the issue. My initial reaction is to doubt this is the actual cause since the only change it made was to execute a pass that has a problem that seems to originate because of a mismatch between module and context. My other initial reaction is to strongly suspect this is actually a SPIRV-Tools bug, but right now the only reproduction I have is with dxc. So we'll adopt it until we find its real parents.

[pow2clk]

The short version is that the change I cited is to blame. The changed getPreservedAnalyses suggests that the config isn't altered by compact IDs pass, but because it mucks with result IDs, which can be used to index basic blocks, that's not true. I've not yet devised a test within SPIRV-Tools to fire this assert, but I will and then I'll submit a PR to that project with the fix and a test to catch any such regression in the future.

Part of the reason this went unnoticed might be that the compact IDs pass is rarely run. The SPIRV optimizer provides functions that register big blocks of passes for different purposes, compact IDs isn't in any of them. It is only run by dxc because it is very explicitly enabled in SPIRVEmitter.cpp citing legalization reasons. It looks like @antiagainst added it. Perhaps he can comment. The SPIRV-Tools code is still wrong, but I wonder if this is needed by dxc.

There is something else that seem wrong in SPIRV-Tools as well. This problem arose in ir_context::CheckCFG() because the config was inconsistent. Usually, the check would be skipped for this pass if it was flagged as not preserving the config. But the cfg() function checks the same flag and will update it if needed. So skipping the check is unnecessary. I've confirmed this with the test in question as well as all the included ctests.

[antiagainst]

Oh, didn't notice this issue. Sorry, @pow2clk! Thanks for noticing this assert and investigate into it! Yes, we explicitly enabled the compat ID pass, because we've heard some drivers favors smaller IDs.

[antiagainst]

And because we are only building release configuration, this is totally missed by bots...

[pow2clk]

@antiagainst Thanks for the clarification. That's good enough for me. There's no harm including it if there's cause except that it doesn't seem to be very well tested, but that's a sample size of one.

It was missed by the DirectXShaderCompiler bots because it is only triggered when dxc is called to generate spirv. clang-spirv-tests doesn't produce this issue. It was by mistakenly using a shiny new untested SPIRV-Tools repo that I discovered this, but it was trying to add a new test of dxc -spirv to the bots that I discovered it had made it in despite my half-hearted effort to prevent it.

I don't know what SPIRV-Tools bots do, but the tests there don't trigger this issue either even on Debug. I mean to change that when I fix it though.

[dneto0]

If you can share a test case please file an issue on SPIRV-Tools.

cc @s-perron

[pow2clk]

@dneto0 working on it! Will do when I can.

[pow2clk]

KhronosGroup/SPIRV-Tools#1648 is opened to address this issue. Since I've confirmed it's entirely related to SPIRV-Tools, I'm closing this issue and moving discussion there. See you on the other side!

[ehsannas]

Thanks @pow2clk