[HLSL] Make a plan for builtin diagnostics

[llvm-beanz]

Many of the HLSL builtins are reporting diagnostics using err_typecheck_convert_incompatible, which produces diagnostics of the form:
error: passing <type1> to parameter of incompatible type <type2>. This diagnostic strongly implies the usage of the builtin requires a specific type, which is not the case. For example see the __builtin_hlsl_cross intrinsic which is valid for 16 and 32-bit floating point types and vectors of such type.

export int4 fn(int4 x, int4 y) {
    return __builtin_hlsl_cross(x, y);
}

export int16_t4 fn(int16_t4 x, int16_t4 y) {
    return __builtin_hlsl_cross(x, y);
}

Compiler Explorer

Diagnostics:

<source>:2:33: error: passing 'int4' (aka 'vector<int, 4>') to parameter of incompatible type '__attribute__((__vector_size__(4 * sizeof(float)))) float' (vector of 4 'float' values)
    2 |     return __builtin_hlsl_cross(x, y);
      |                                 ^
<source>:6:33: error: passing 'int16_t4' (aka 'vector<int16_t, 4>') to parameter of incompatible type '__attribute__((__vector_size__(4 * sizeof(float)))) float' (vector of 4 'float' values)
    6 |     return __builtin_hlsl_cross(x, y);
      |                                 ^

These diagnostics strongly indicate to a user that you must call the builtin with a float4.

Related: llvm/llvm-project#125673

Our strategy for diagnosing builtins for HLSL should align with Clang's general approach.

[farzonl]

Moving this conversation here.

we should be able to emit a diagnostic that the arguments must be "16-bit or 32-bit floating point or vectors of such types"

The diagnostic string suggestion as is would be difficult to make configurable. If we don't intend them to be configurable then we would be adding potentially multiple new diagnostics to cover all the helpers that are using CheckArgTypeIsCorrect. An alteration to take multiple expected types would break down because some cases are just two overloads in float\half others cover all 16 to 32 bit float and integers.

Check	New Diag
CheckFloatOrHalfRepresentations	"16-bit or 32-bit floating point or vectors of such types"
CheckAllArgsHaveFloatRepresentation	"Any floating point or vectors of such types"
CheckNoDoubleVectors	"Any floating point or vectors of 16-bit or 32-bit floating point"
CheckUnsignedIntRepresentation	"Any unsigned integer or vectors of such types"
CheckFloatingOrIntRepresentation	"Any floating point or integer or vectors of such types"

We don't have to maintain these five cases, but im croncicalizing them here so we know what needs to be replaced.

I think the simplest thing is to just do what overloads does and emit err_ovl_ambiguous_call

static bool CheckArgTypeIsCorrect(
    Sema *S, Expr *Arg,
    llvm::function_ref<bool(clang::QualType PassedType)> Check) {
  QualType PassedType = Arg->getType();
  if (Check(PassedType)) {
    S->Diag(Arg->getBeginLoc(), diag::err_ovl_ambiguous_call);
    return true;
  }
  return false;
}

If we want to specify specific expected types maybe they should be notes?

static bool CheckArgTypeIsCorrect(
    Sema *S, Expr *Arg, llvm::SmallVectorImpl<QualType> ExpectedTypes,
    llvm::function_ref<bool(clang::QualType PassedType)> Check) {
  QualType PassedType = Arg->getType();
  if (Check(PassedType)) {
    S->Diag(Arg->getBeginLoc(), diag::err_ovl_ambiguous_call);
    for (const clang::QualType &ExpectedType : ExpectedTypes)
      // some logic to emit the notes.
    return true;
  }
  return false;
}

[llvm-beanz]

Builtin functions are not called and they are not overloads, so we should not be using err_ovl_ambiguous_call.

[llvm-beanz]

DXC effectively had two disagreeing implementations of overload resolution between user-defined function resolution and HLSL library function resolution. We shouldn't be aiming to repeat that mistake in part or in whole by treating builtins as functions that are resolved.

We shouldn't be replicating overload resolution behavior or diagnostics for builtins because they are not resolved functions.

We should validate that each builtin call has a valid set of arguments, and if it doesn't we should emit a meaningful diagnostic that helps users understand their mistake. In the common case the users of these builtins will be our team in the future, not end users. Since these are implementation details we can strike a balance between complexity and completeness. Personally I think noting every possible combination of arguments that would be valid is a bit overkill, I'd rather we have simple diagnostics like:

• all arguments must be the same type
• arguments two and three must be the same type
• argument must be of type or vector of such type
• argument must be of {32-bit or 16bit|} {floating point|integer} type or vector of such type

This form of diagnostic matches with other diagnostics clang emits for builtin functions, it is clear to a user the range of supported types, and it is not overly noisy by listing a pile of notes of potentially valid types.

Whether or not this works with the coding pattern currently employed is irrelevant to the discussion. If the pattern works and produces the results we want that's great, if the pattern does not produce the results we want it isn't serving its need and we should re-evaluate how we are processing arguments.

Having a pattern that is easy to follow for onboarding teammates is only useful if the pattern produces the results we want, otherwise we're training new teammates to build the wrong thing following patterns that don't produce the desired results.

[llvm-beanz]

Related: llvm/llvm-project#125673

Our strategy for diagnosing builtins for HLSL should align with Clang's general approach.

[spall]

Proposed strategy for sema checking builtins:

1. Check for the correct number of arguments:
2. Check that each argument is an appropriate type
3. Check that all arguments/some arguments are the same type (if appropriate)
4. For certain math functions, check additional math restrictions

• For 1, Error messages will remain the same as they are now.
• For 2, Error messages, in most cases, will be produced using the following diagnostics

1. err_typecheck_expect_scalar_or_vector (A scalar or vector of one type is allowed)
2. err_typecheck_expect_any_scalar_or_vector (A scalar or vector of ANY type is allowed)
3. err_builtin_invalid_arg_type (This err is customizable and will be used in most cases)

Addressing @llvm-beanz concern about error messages for '__builtin_hlsl_cross', which has an unusual type signature, using this new method the proposed error messages would be

1st argument must be a vector floating-point type (was 'int4' (aka 'vector<int, 4>'))
And
1st argument must be a vector floating-point type (was 'int16_t4' (aka 'vector<int16_t, 4>'))

Potential limitations of err_builtin_invalid_arg_type

• Does not have a bool option, but most hlsl instrinsics do not support bool
• No option to specify the bitsize of integers or floating point, but it does look possible to add something like '16 or 32 bit floating point', without disturbing existing tests using this err
• No option which says 'scalar or vector or matrix', but it does look possible to add such an option without disturbing existing tests which use this err

For 3, Error messages will be of the form 'All arguments to X must have the same type', and it seems possible this error message can be enhanced to support errors of the form: 'Second and Third argument to X must have the same type', if necessary.

[damyanp]

@spall - please create appropriate issues tracking the implementation of this plan, then we can close this one.