@anupamachandra, do you think your work on #5358 is going to overlap with this?

Yes, I believe so.

@llvm-beanz I thought the issue was that the subscript operator for NodeOutputArray was defined as `T& operator versus T operator. That is not the case, it does return a value:

`-DeclRefExpr 0x21b5817eb70 <col:42, col:44> 'NodeOutput<Record> (unsigned int)' lvalue CXXMethod 0x21b5817e818 'operator[]' 'NodeOutput<Record> (unsigned int)'

Also, #5357 requires adding type annotations to reference types returned by intrinsics, with that in place, should this be changed to return a reference, consistent with the other HLSL types.

@tex3d may need to weigh in here. I know that I authored the public version of this issue, but Tex authored the original internal issue (111).

Tex had an internal comment:

The annotation may be missing because our code paths that add it aren't triggered for the an intermediate NodeOutput object in some cases, for example myNodeOutputArray[i] won't get a type annotation added because there's no explicit intermediate variable, because the operator[] returns a reference:

ThreadNodeOutputRecords<MyRecord> myRecords =
 myNodeOutputArray[i].GetThreadNodeOutputRecords(...);

But it would if you wrote it like this:

 NodeOutput<MyRecord> myOutput = myNodeOutputArray[i];
 ThreadNodeOutputRecords<MyRecord> myRecords =
 myOutput.GetThreadNodeOutputRecords(...);

Now, operator[] shouldn't return a reference in this case (which is one part of a fix, but I think that one wasn't trivial), and we also need to cover the reference case for adding the type annotation anyway.

We may just need testing to verify that these cases are correctly handled by the code in CGHLSLMSFinishCodeGen.cpp that creates node record handle annotations.

@tex3d thoughts?

The issue was detected when the NodeOutputArray subscript operator returned a reference to a NodeOutput object, which was not correct. For returning the record struct itself (not the object), like what Get() and the Node*OutputRecords subscript operators return, these should be a reference. But when returning an object (handle), it should be a value (the object is synthesized, not pre-existing and referenced).

Now, the subscript operator was fixed, but the bug that can lead to a missing type annotation remains which might impact some other operator that returns a reference.

That's the issue: find what else this type annotation gap might impact, write a test for that, and fix it.

To be clear, this no longer impacts the same work graph case, it might impact some other pre-existing scenario, it isn't a regression, therefore shouldn't be considered an SM 6.8 issue.

In light of the above, this is an existing issue that was not introduced by 6.8 work nor does it impact any 6.8 features insofar as we are aware. As such, I'll be dropping this from the sm6.8 milestone.

@anupamachandra

@llvm-beanz I thought the issue was that the subscript operator for NodeOutputArray was defined as `T& operator versus T operator. That is not the case, it does return a value:

`-DeclRefExpr 0x21b5817eb70 <col:42, col:44> 'NodeOutput<Record> (unsigned int)' lvalue CXXMethod 0x21b5817e818 'operator[]' 'NodeOutput<Record> (unsigned int)'

Also, #5357 requires adding type annotations to reference types returned by intrinsics, with that in place, should this be changed to return a reference, consistent with the other HLSL types.

To directly answer this question: this operator is now correct.

It should return a value, since it's not returning a reference to an existent thing elsewhere, instead it's synthesizing a handle that refers to a particular output slot. The references you see for other subscript operators are returning references to data in a buffer or other existent things, which are meant to be read and/or modified directly.

This particular issue was just about the possibility of a missing type annotation which might occur elsewhere. It should be explored, but it's also possible that the issue is not reachable at this time.

@tex3d
Thank you for the details. I went back to the original change that added the subscript operator for NodeOutputArray types and it returns a value and not a reference, that part of the code hasn't changed. It works now because we added a recordSizeWAR code in AddOpcodeParamForIntrinsic function in CGHLSLMSFinishCodeGen.cpp. Without the workaround, we see failures from missing type annotation like before. I can leave this assigned to myself and work on it in a few weeks if you think that this fix is not critical for the SM6.8 release.

@tex3d Thank you for the details. I went back to the original change that added the subscript operator for NodeOutputArray types and it returns a value and not a reference, that part of the code hasn't changed. It works now because we added a recordSizeWAR code in AddOpcodeParamForIntrinsic function in CGHLSLMSFinishCodeGen.cpp. Without the workaround, we see failures from missing type annotation like before. I can leave this assigned to myself and work on it in a few weeks if you think that this fix is not critical for the SM6.8 release.

I thought there was some intermediate version (may never have been merged) that returned a reference which I was debugging around the time I filed the original issue. I distinctly remember that in codegen, direct use of the reference return value of the subscript operator would not generate the temp, so it wouldn't add the type annotation, while an intermediate object returned from the operator would generate a temp (RValue), which caused the type annotation to be added. I remember being able to work around the issue by assigning the result from the subscript operator to a local temp variable, then using the temp variable instead. Perhaps there's another interacting factor here as well. Oh well...

In any case, I think the missing type annotation issue is a problem, but not one blocking the release at the moment. So, I think your suggestion to keep it assigned to yourself and work on it in a few weeks would be fine.

Dang, it looks like type annotations are still not added for these subscript operators when they return values as well.

I was able to crash the current code due to a missing annotation still with this:

struct RECORD1 {
    uint value;
};

[Shader("node")]
[NodeLaunch("broadcasting")]
[NodeDispatchGrid(1, 1, 1)]
[NumThreads(128, 1, 1)]
void node_1_1(
    [NodeArraySize(128)] [MaxRecords(64)] NodeOutputArray<RECORD1> OutputArray
) {
    OutputArray[1].GetThreadNodeOutputRecords(2).OutputComplete();
}

@anupamachandra
I put up a draft PR that adds missing type annotations by iterating methods on node objects, as well as making sure that any Type template parameter for something we are adding template argument annotation for will also have a type annotation added.

@tex3d Thank you for the details. I went back to the original change that added the subscript operator for NodeOutputArray types and it returns a value and not a reference, that part of the code hasn't changed. It works now because we added a recordSizeWAR code in AddOpcodeParamForIntrinsic function in CGHLSLMSFinishCodeGen.cpp. Without the workaround, we see failures from missing type annotation like before. I can leave this assigned to myself and work on it in a few weeks if you think that this fix is not critical for the SM6.8 release.

I thought there was some intermediate version (may never have been merged) that returned a reference which I was debugging around the time I filed the original issue. I distinctly remember that in codegen, direct use of the reference return value of the subscript operator would not generate the temp, so it wouldn't add the type annotation, while an intermediate object returned from the operator would generate a temp (RValue), which caused the type annotation to be added. I remember being able to work around the issue by assigning the result from the subscript operator to a local temp variable, then using the temp variable instead. Perhaps there's another interacting factor here as well. Oh well...

In any case, I think the missing type annotation issue is a problem, but not one blocking the release at the moment. So, I think your suggestion to keep it assigned to yourself and work on it in a few weeks would be fine.

@tex3d you are right, the original PR had the subscript operator returning a reference that was fixed with using a temp which invoked the AddTypeAnnotation path in EmitAutoVar. I had fixed the subsctript operator return but the underlying issue remained. Thanks for the fix, I'll take a look at it.