Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Improvements to builtin typenames handling.

Merged Nikolay Sivov requested to merge nsivov/vkd3d:hlsl_typenames into master
4 unresolved threads

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • From patch 1/4:

      +static const char * get_case_insensitive_typename(const char *name)
      +{
      +    static const char *names[] =
      +    {
      +        "dword",
      +        "float",
      +        "matrix",
      +        "string",
      +        "vector",
      +    };

      We'd want the names[] array itself to be const as well, right? (I.e., "static const char *const names[] = ...")

    • Author Developer

      Right, pushed that.

    • Please register or sign in to reply
  • Nikolay Sivov added 4 commits

    added 4 commits

    • fc7b1907 - vkd3d-shader/hlsl: Support case-insensitive lookup for selected builtin types.
    • 0f83dbeb - vkd3d-shader/hlsl: Use unsigned type for "dword" alias.
    • 79214f6b - vkd3d-shader/hlsl: Initial support for string constants.
    • 56b67c4d - vkd3d-shader/hlsl: Allow case-insenstive names for shader objects types.

    Compare with previous version

  • I don't think this approach is quite going to work. Consider the following shader:

    typedef float2 Dword;

    float4 main() : sv_target { DWORD f = 1.0; return f; }

    It seems that redefining these types is legal (although it doesn't seem to be legal to redefine lowercase "dword" as a type?). E.g. that shader compiles as-is, but if the first line is changed to "DWORD" it will predictably fail (float2 to float4 implicit conversion).

    It also seems that at least "vector" and "string" should be case-insensitive keywords rather than typedefs. That's mostly out of scope here, but it does mean that patch 3/4 is going in the wrong direction.

    From patch 3/4:

    @@ -3509,6 +3509,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl INT intval; FLOAT floatval; bool boolval;

    • char *strval; char *name; DWORD modifiers; struct hlsl_ir_node *instr;

    I'd just rename "name" rather than adding a new union value.

    @@ -611,6 +611,7 @@ struct hlsl_ir_constant int32_t i; float f; double d;

    •    const char *s;
      } value[4];

    This is going to need special handling when the constant is cloned or freed.

  • Author Developer

    Yes, looks like it's more convoluted. "vector" and "string" are lowercase tokens that can't be used for variable names for instance. Doing "float2 sTring;" is valid, but "sTring" can't be used as a type after that. Reusing names of already declared variables as typenames is universally disallowed, so that's good.

  • Nikolay Sivov added 18 commits

    added 18 commits

    • 56b67c4d...02e3be81 - 15 commits from branch wine:master
    • 200b2cc8 - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'dword' type.
    • e7e6c6dc - vkd3d-shader/hlsl: Use unsigned type for the 'dword' alias.
    • a790ca79 - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'float' type.

    Compare with previous version

  • Author Developer

    Removed string constants part for now until it's figured out (it was hitting fatal fixme for "uniform initializer" anyway).

    Updated changes better reflect what's going on.

  • Yes, looks like it's more convoluted. "vector" and "string" are lowercase tokens that can't be used for variable names for instance. Doing "float2 sTring;" is valid, but "sTring" can't be used as a type after that. Reusing names of already declared variables as typenames is universally disallowed, so that's good.

    Hrm, I thought I tested redefining non-lower-case versions of those, but I must have done it wrong.

    Anyway, this is still failing with the example I posted, as far as I can tell because hlsl_get_type's recursive call passes "false" rather than propagating the "case_insensitive" argument.

    Also, from patch 1/3:

    • for (i = 0; i < ARRAY_SIZE(names); ++i)
    •    if (!ascii_strcasecmp(names[i], name)) return names[i];

    Please put the "if" body on a new line, and use braces for multi-line bodies even if it's technically a single statement.

    • if (case_insensitive && !scope->upper && (name = get_case_insensitive_typename(name)))
    • {
    •    if ((entry = rb_get(&scope->types, name)))
    •        return RB_ENTRY_VALUE(entry, struct hlsl_type, scope_entry);
    • }

    It looks mildly wrong that this checks "scope->upper" but not "recursive". It works, because we never pass recursive == false but case_insensitive == true, but I think it'd be clearer just to move this below the recursive call or use "else" there.

    @@ -4565,7 +4565,7 @@ type_no_void: } | KW_STRUCT TYPE_IDENTIFIER {

    •        $$ = hlsl_get_type(ctx->cur_scope, $2, true);
    •        $$ = hlsl_get_type(ctx->cur_scope, $2, true, false);
             if ($$->type != HLSL_CLASS_STRUCT)
                 hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_REDEFINED, "\"%s\" redefined as a structure.", $2);
             vkd3d_free($2);

    This lacks tests (and also feels a little suspicious, mostly because it contradicts the lexer).

  • Nikolay Sivov added 4 commits

    added 4 commits

    • d86db8bc - 1 commit from branch wine:master
    • ab558d44 - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'dword' type.
    • bae9972d - vkd3d-shader/hlsl: Use unsigned type for the 'dword' alias.
    • 581fc69f - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'float' type.

    Compare with previous version

  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit ab558d44
543 543 }
544 544
545 struct hlsl_type *hlsl_get_type(struct hlsl_scope *scope, const char *name, bool recursive)
545 static const char * get_case_insensitive_typename(const char *name)
546 {
547 static const char *const names[] =
548 {
549 "dword",
550 };
551 unsigned int i;
552
553 for (i = 0; i < ARRAY_SIZE(names); ++i)
554 if (!ascii_strcasecmp(names[i], name))
555 {
556 return names[i];
557 }
    • I guess I didn't quite think the struct tests through; I forgot we use a separate rule to actually define struct types, and so the automated test doesn't prove anything one way or another. A manual test confirms that v4 is correct, though, at least.

      In my defense, though, that error message is wrong and misled me; we can't be "redefining" anything in that rule. I'll send a patch to fix it.

    • Author Developer

      Maybe it's worded like that because of the error message that Windows produces - "redefinition of type with struct/class". It's probably a side effect of something else happening in type name collision, obviously this can't define a new type. Using unique name will give another error right away.

    • Ah, that'd make sense.

    • Please register or sign in to reply
  • Nikolay Sivov added 3 commits

    added 3 commits

    • eb02f7fd - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'dword' type.
    • 2507a8e6 - vkd3d-shader/hlsl: Use unsigned type for the 'dword' alias.
    • 3a5fc679 - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'float' type.

    Compare with previous version

  • Nikolay Sivov added 3 commits

    added 3 commits

    • 1c413866 - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'dword' type.
    • 7b1f8272 - vkd3d-shader/hlsl: Use unsigned type for the 'dword' alias.
    • ae7e5d41 - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'float' type.

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • added 18 commits

    • ae7e5d41...5f904e50 - 15 commits from branch wine:master
    • 89121766 - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'dword' type.
    • df2d6d35 - vkd3d-shader/hlsl: Use unsigned type for the 'dword' alias.
    • dd36215a - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'float' type.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading