Assertion reached in SEMA related to builtin template specialization

[Keenuts]

Hi,

Reached this assert while trying to reproduce #4856

revision: 2c3d965
command line: ./build/bin/dxc -T cs_6_0 -E main repro.hlsl

code:

[[vk::binding(0)]] Texture2D<float> g_depth_buffer : register(t0);

#define A_GPU
#define A_HLSL
#include "ffx_a.h"

groupshared float g_group_shared_depth_values[16][16];

AF4 SpdLoadIntermediate(AU1 x, AU1 y) {
	float f = g_group_shared_depth_values[x][y];
	return f.xxxx;
}


[numthreads(32, 8, 1)]
void main(uint3 dispatch_thread_id : SV_DispatchThreadID, uint3 group_id : SV_GroupID, uint group_index : SV_GroupIndex){
}

I do not have a ffx_a.h file on my machine, at least not that I know of. Removing the first line makes DXC fails with the "ffx_a.h not found".

Error:

dxc: SemaHLSL.cpp:788: clang::QualType GetOrCreateTemplateSpecialization(clang::ASTContext &, clang::Sema &, clang::ClassTemplateDecl *, ArrayRef<clang::TemplateArgument>): Assertion `false == sema.InstantiateClassTemplateSpecialization( NoLoc, specializationDecl, TemplateSpecializationKind::TSK_ImplicitInstantiation, true)' failed.
Aborted

[pow2clk]

Not limited to SPIRV. even without the [[vk::binding]] annotation and targetting DXIL, this is an issue.

@tex3d speculates that it is likely failing to completely abort when reaching the failure of the missing include. Instead, it continues to try to compile skipping crucial steps so it hits this assert.

The shader is pretty fiddly, but I was able to reduce it a bit:

Texture2D<float> g_depth_buffer : register(t0);

#include "nonexistent.h"

groupshared float g_group_shared_depth_values[16][16];

float LoadGS(float x, float y) {
	float f = g_group_shared_depth_values[x][y];
	return f.xxxx;
}

void main(){}

compile as -T vs_6_0

Removing almost any of these lines except for the last one will remove the repro.

[llvm-beanz]

The issue reproduces on the release-1.7.2207 tag, so this is not new and is unrelated to the change replacing exceptions in the FileIO path.

[llvm-beanz]

I think I see what is happening here...

The assert that is firing is caused by failing to instantiate the vector type created by the f.xxxx. The instantiation seems to be failing here. Which I think is the caused by the include failure.

The failed include causes the instantiating template to be marked as invalid, which results in the instantiation failing. We then assert on the failed instantiation instead of reasonably propagating the error.

The method the assert is in is not written to propagate errors, so we should fix this by refactoring that method and its call sites to propagate errors properly, which may take some work.

[harshpg]

@llvm-beanz

I'm curious why [[vk::binding(0)]] Texture2D<float> g_depth_buffer : register(t0); has an effect on the assertion behavior. Removing this line makes the missing include be handled correctly.

[llvm-beanz]

HLSL's built-in templates (like Texture2D) aren't fully formed AST types. To make them work and go through instantiation there's a lot of special logic that only applies (or should only apply) for those types. The assert being hit is in the logic that instantiates the resource template. If you have no resource template declarations in the file you won't hit the assert because you don't execute the function.

The bug is that the HLSL-specific code for instantiating resource templates assumes that after DXC has already validated the template parameters through its own logic the actual AST-level instantiation can't fail. Which isn't a wholly unreasonable assumption, but it is a false assumption.

The failure occurs because in C++ template instantiation can be really slow, and the resolution rules can vary contextually. Because of those two reasons, Clang returns failure (without a specific error) when instantiating a template after a fatal error has occurred in the compilation. This decision prioritizes giving an actionable error fast rather than spewing cascading nonsensical errors that are common in template instantiation errors, and is pretty defensibly the right decision.

The end result is that if the include fails, the failure is identified and logged correctly, but when the template instantiation fails we hit an assert (which depending on platform and build settings either becomes a hard error or works as expected).

IMO, the core issue here is one of best practices. I don't think it is ever a good programming practice to ignore a possible error return. Some exceptions can be made in cases where you're 100% confident you've verified that the error can't possibly happen (through sufficient checking ahead of the call), but we should be exceedingly careful when we do that. Ignoring error returns from complicated pieces of logic like template instantiation is probably never a good idea due to the underlying complexity.

[llvm-beanz]

This should be fixed by the associated PR.

[llvm-beanz]

Assigned to @tcorringham since his PR should resolve this.