PIX:Descend Type Hierarchy for Alloca'd structs

[jeffnn]

In some cases structs like the RayDesc built-in type (one of the arguments to TraceRays) will be placed in an alloca.
The PIX annotation code wasn't properly counting all the leaf-node values of aggregates within such structs when determining offsets for later members.
The result of this was erroneous attribution of writes to those members during PIX shader debugging.

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.2822 completed (commit 9dc6db4c3c by @jeffnn)

[llvm-beanz]

Please include a test case.

[jeffnn]

Please include a test case.

Fair enough! This would be well-tested on the PIX side, but that's no reason not to have something here.

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.2827 completed (commit 1cac2721e4 by @jeffnn)

[llvm-beanz]

Fair enough! This would be well-tested on the PIX side, but that's no reason not to have something here.

I realize we haven't historically enforced this requirement as rigorously as we should, but our contribution guidelines do require that every change has a test case.

I am looking into adding automatic verification of that in our PR testing as well.

[llvm-beanz]

I don't see anything obvious about this change that is problematic from the LLVM perspective. Whether or not it is correct for PIX I assume you're testing separately. It would be nice to have a simple unit test for this code that verifies the IR metadata generation. We have very few tests that verify the IR results of the PIX passes.

Adding a test file under tools/clang/test/HLSLFileCheck/pix that contained something like this would be useful to verify the correct metadata generation in the IR:

// RUN: %dxc -Od -T lib_6_6 %s | %opt -S -dxil-annotate-with-virtual-regs | FileCheck %s


/* To run locally run:
%dxc -Od -T lib_6_6 %s -Fc %t.ll
%opt %t.ll -S -dxil-annotate-with-virtual-regs | FileCheck %s
*/

RaytracingAccelerationStructure scene : register(t0);

struct RayPayload
{
    int3 color;
};

[shader("raygeneration")]
void ENTRY()
{
    RayDesc ray = {{0,0,0}, {0,0,1}, 0.05, 1000.0};
    RayPayload pld;
    TraceRay(scene, 0 /*rayFlags*/, 0xFF /*rayMask*/, 0 /*sbtRecordOffset*/, 1 /*sbtRecordStride*/, 0 /*missIndex*/, ray, pld);
}

// CHECK: {{.*}} = alloca %struct.RayDesc, align 4, !pix-dxil-inst-num {{.*}}, !pix-alloca-reg [[RDAlloca:![0-9]+]]
// CHECK: {{.*}} = alloca %struct.RayPayload, align 4, !pix-dxil-inst-num {{.*}}, !pix-alloca-reg [[RPAlloca:![0-9]+]]
// CHECK: {{.*}} = getelementptr inbounds %struct.RayDesc, %struct.RayDesc* {{.*}}, i32 0, i32 0, !pix-dxil-inst-num {{.*}}, !pix-dxil-reg [[RDGEP:![0-9]+]]
// CHECK: {{.*}} = load i32, i32* getelementptr inbounds ([1 x i32], [1 x i32]* @dx.nothing.a, i32 0, i32 0), !pix-dxil-inst-num {{.*}}, !pix-dxil-reg [[NothGEP:![0-9]+]]
// CHECK: {{.*}} = getelementptr inbounds %struct.RayDesc, %struct.RayDesc* {{.*}}, i32 0, i32 1, !pix-dxil-inst-num {{.*}}, !pix-dxil-reg [[RDGEP2:![0-9]+]]
// CHECK: {{.*}} = load i32, i32* getelementptr inbounds ([1 x i32], [1 x i32]* @dx.nothing.a, i32 0, i32 0), !pix-dxil-inst-num {{.*}}, !pix-dxil-reg [[NothGEP2:![0-9]+]]
// CHECK: {{.*}} = getelementptr inbounds %struct.RayDesc, %struct.RayDesc* {{.*}}, i32 0, i32 2, !pix-dxil-inst-num {{.*}}, !pix-dxil-reg [[RDGEP3:![0-9]+]]
// CHECK: {{.*}} = load i32, i32* getelementptr inbounds ([1 x i32], [1 x i32]* @dx.nothing.a, i32 0, i32 0), !pix-dxil-inst-num {{.*}}, !pix-dxil-reg [[NothGEP3:![0-9]+]]

// CHECK-DAG: [[RDAlloca]] = !{i32 1, i32 0, i32 8}
// CHECK-DAG: [[RPAlloca]] = !{i32 1, i32 8, i32 3}
// CHECK-DAG: [[RDGEP]] = !{i32 0, i32 0}
// CHECK-DAG: [[NothGEP]] = !{i32 0, i32 11}
// CHECK-DAG: [[RDGEP2]] = !{i32 0, i32 3}
// CHECK-DAG: [[NothGEP2]] = !{i32 0, i32 12}
// CHECK-DAG: [[RDGEP3]] = !{i32 0, i32 4}
// CHECK-DAG: [[NothGEP3]] = !{i32 0, i32 13}

An alternate testing strategy we could consider in the future would be building a pretty-printer that translated the IR metadata into a more human print format, but in the absence of that testing the raw IR is better than nothing.

[jeffnn]

Hey thanks for the assist. I didn't know about that syntax for testing metadata nodes. Cool!

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.2828 completed (commit 4df8dcb0ed by @jeffnn)