Fix the return types of dot4add_i8packed and dot4add_u8packed.

[jimblandy]

Change the definition of the HLSL dot4add_i8packed and dot4add_u8packed intrinsics in utils/hct/gen_intrin_main.txt to simply spell out the return types, rather than saying that their return type is determined by their third argument.

This prevents DXC from trying to give those functions declarations like

declare i64 @"\01?dot4add_u8packed@hlsl@@YA_JII_J@Z"(i32, i32, i64 signext) #1

which seems to expect a 64-bit third argument and return value.

These functions are not generic, and they have only one overload, so there is no need to use interesting uComponentTypeId values to get the right effects, and HLSLExternalSource::MatchArguments seems to get confused about how to treat argument types that affect the return types.

Fixes #7400.

[jimblandy]

I have absolutely no idea what I'm doing. Any advice would be welcome.

[jimblandy]

@amaiorano Hi! This is another PR that seems to fix a bug that Firefox runs into.

[jimblandy]

Added a test.

[amaiorano]

@amaiorano Hi! This is another PR that seems to fix a bug that Firefox runs into.

I've added DXC devs from Microsoft who would know this code better than me.

[pow2clk]

Fix looks good. I'd like to see it expanded to another place it is needed and testing to cover it all, but otherwise I'm happy!

By way of a little more explanation since I took the time to figure out why this did what it did, we have a special optimization path for intrinsics whose return types depend on their arguments where all are immediates. This is where it will now correctly resolve the immediate because that dependency is removed:

DirectXShaderCompiler/tools/clang/lib/Sema/SemaHLSL.cpp

Lines 6738 to 6747 in 1198c30

	// For literal arg which don't affect return type, find concrete type.
	// For literal arg affect return type,
	// TryEvalIntrinsic in CGHLSLMS.cpp will take care of cases
	// where all argumentss are literal.
	// CombineBasicTypes will cover the rest cases.
	if (!affectRetType) {
	TypeInfoEltKind =
	ConcreteLiteralType(pCallArg, TypeInfoEltKind,
	pIntrinsicArg->uLegalComponentTypes, this);
	}

This is where in other functions that match this signature, a potential optimization reduces them to a constant expression:

DirectXShaderCompiler/tools/clang/lib/CodeGen/CGHLSLMSFinishCodeGen.cpp

Line 2011 in 1198c30

Value *TryEvalIntrinsic(CallInst *CI, IntrinsicOp intriOp,

As you can see there, the dot products are not included as they can't be a constant expression, so it never gets resolved properly.

That's why I've concluded that this is an acceptable change and it doesn't reveal any deeper issue apart from a brittle optimization opportunity detection that I don't think is worth addressing.

[jimblandy]

we have a special optimization path for intrinsics whose return types depend on their arguments where all are immediates.

Okay, great. I noticed this path getting taken, and that gave me the idea of changing the types in gen_intrin_main.txt. I'm glad to have someone who knows what's going on double-check this.

[jimblandy]

@pow2clk Thanks very much for the review. I think the PR now addresses your comments.

[pow2clk]

@jimblandy the follow up review would have been a lot easier if you'd not force pushed the changes. I understand you wanted to rebase, but you can do that with a merge which will keep those changes separate and would have allowed me to just review the new commit.

[pow2clk]

Thanks for the changes!

[jimblandy]

@jimblandy the follow up review would have been a lot easier if you'd not force pushed the changes. I understand you wanted to rebase, but you can do that with a merge which will keep those changes separate and would have allowed me to just review the new commit.

I'm sorry - I'll be sure to do this next time.