Prevent duplicate array type insertion into TypeMap.

[maarquitos14]

Check if an array type exists in TypeMap before inserting it to prevent duplications.

[maarquitos14]

@svenvh can you take a look too?

[svenvh]

As far as I understand, the point of the assertion is not to prevent duplication in the map itself, but to warn if duplication would have happened. This was added by @MrSidims in #1047 so it would be good to have his opinion too.

That said, I think this might have been an issue with non-opaque pointers only, which is now past us, so maybe we indeed no longer need this check?

[maarquitos14]

As far as I understand, the point of the assertion is not to prevent duplication in the map itself, but to warn if duplication would have happened. This was added by @MrSidims in #1047 so it would be good to have his opinion too.

That said, I think this might have been an issue with non-opaque pointers only, which is now past us, so maybe we indeed no longer need this check?

@MrSidims is OOO for a few days now. If no strong opposition, I'd like to have this merged and follow up with him later, if he has any concerns.

[MrSidims]

I'll try to remind myself why this commented assertion was added, but basically the current status is "the type mapping issue" (whatever it is) is not resolved.

[MrSidims]

Btw, llvm-spirv pointer types map is currently bugged. And I'm the one who has introduced it in https://github.com/KhronosGroup/SPIRV-LLVM-Translator/pull/2650/files#diff-8865333527b4a0615ba95940c2a69e7a9607b6dfc696408bb9501d38aa762569R1008 <- there is no SPIR-V spec restriction of not having 2 or more identical OpTypePointer instructions in the module. More over, it is expected behaviour in cases, when they are decorated differently. Not sure if it relates with the issue we have recently observed.

[maarquitos14]

Btw, llvm-spirv pointer types map is currently bugged. And I'm the one who has introduced it in https://github.com/KhronosGroup/SPIRV-LLVM-Translator/pull/2650/files#diff-8865333527b4a0615ba95940c2a69e7a9607b6dfc696408bb9501d38aa762569R1008 <- there is no SPIR-V spec restriction of not having 2 or more identical OpTypePointer instructions in the module. More over, it is expected behaviour in cases, when they are decorated differently. Not sure if it relates with the issue we have recently observed.

At least one of the issues we have recently observed triggers when calling mapType on id 307, on something like the code below. Note that ids 28 and 307 are the same, and I don't see any decorations on neither of them.

TypeInt 3 8 0
TypePointer 27 8 3

TypeArray 28 27 26
TypePointer 29 7 28

TypeArray 307 27 26
TypeStruct 306 307
TypePointer 308 7 306

[maarquitos14]

Digging a little bit deeper, the error comes from
https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/lib/SPIRV/SPIRVWriter.cpp#L464.

The first array (Id 28 of the code of my previous comment) has the following LLVM Type: [2 x typedptr(i8, 4)], while the second array (Id 307 of the code of my previous comment) has the following LLVM Type: [2 x ptr addrspace (4)]. They aren't the same type, but when we take the branch of the line I just referenced above, it calls mapType with T = [2 x typedptr(i8, 4)]. As you can see, we never check in this case if the map already contains this type, so there's the duplication.

I think we can solve this by adding a check in this branch to see if the type already exists in the map before calling mapType. What do you think? Would that be a better solution than removing the assert? @svenvh @MrSidims @vmaksimo
I tested this locally, and it fixes the error we've observed recently.

[vmaksimo]

I think we can solve this by adding a check in this branch to see if the type already exists in the map before calling mapType.

@maarquitos14 makes total sense to me, let's try this approach :)
Please also add a simple test case which will crash without the fix you suggested.

[maarquitos14]

@svenvh @vmaksimo I have updated the PR with the agreed resolution and a test that fails without this patch, but passes with it.

[svenvh]

LGTM, thanks!