Skip to content
Snippets Groups Projects

hlsl: Function calls.

Merged Elizabeth Figura requested to merge zfigura/vkd3d:pr1 into master
2 unresolved threads

Merge request reports

Checking pipeline status.

Merged by Alexandre JulliardAlexandre Julliard 2 years ago (Jan 13, 2023 9:23pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
1162 1162 FIXME("Unhandled type %s.\n", hlsl_node_type_to_string(node->type));
1163 1163 return 0;
1164 1164
1165 case HLSL_IR_CALL:
1165 1166 case HLSL_IR_IF:
1166 1167 case HLSL_IR_JUMP:
1167 1168 case HLSL_IR_LOOP:
1168 1169 case HLSL_IR_RESOURCE_STORE:
1169 1170 case HLSL_IR_STORE:
1170 WARN("Invalid node type %s.\n", hlsl_node_type_to_string(node->type));
1171 assert(0);
  • Maybe this could be updated to vkd3d_unreachable();.

  • Maybe, but the invariant here is "an hlsl_block resulting from an expression should never have one of these as the last instruction, because none of them have a value", which feels more like "invariant that could be violated by accident" than "invariant that can only be violated due to memory corruption". Granted, I think Henri has argued that the former should be ERR() and the latter unreachable(), i.e. there's no room for assert() anywhere...

  • I don't see a real difference between violating an invariant by accident or by cosmic ray. To me any program configuration either can be reached for some user input (which is legitimate, i.e., not considered to cause undefined behavior) or it can't. In the first case an error message must be outputted, in the second case an assertion must be triggered (and vkd3d_unreachable() is just a better assert(0)).

    Just my two cents, though.

  • To me, the difference is: invariants which might be violated by accident deserve to be written in the code to remind the reader of the invariant, and they deserve a warning to be printed if they're violated. By contrast, invariants which can only be violated via memory corruption should, in theory, not even be written in the code—simply because memory corruption can happen anywhere and it's a waste of time to write those assertions. In my opinion the only reason we have vkd3d_unreachable() in things like switch default cases is that the compiler would warn otherwise.

    I did forget that vkd3d_unreachable() actually effectively includes an assert, though; I thought it was always equivalent to __builtin_unreachable().

    Maybe both types should be vkd3d_unreachable(), though, as it is. I.e. if you're a user who trusts the programmer to not violate their own invariants, and you want the most efficient code...

  • Yeah, in my intention vkd3d_unreachable() is equivalent to assert(0), so it aborts with an error message without NDEBUG and it does nothing with NDEBUG, but with the difference that it lets the compiler knows (either via __builtin_unreachable() or via __attribute__((noreturn)) that that piece of code is not meant to be reached (mostly so that the compiler spares us with its overzealous warnings without us sticking in dummy return 0 statements; in theory there is also a performance gain, but I doubt that's ever going to be visible).

    I say that I don't see a real difference between violating an invariant by accident or by memory corruption because in practice I don't know how to differentiate between the two in advance.

  • After thinking about it I'm inclined to agree, there's not really any point in not using vkd3d_unreachable() for this.

    I think part of my hesitation right now, honestly, is that vkd3d_unreachable() doesn't look like assert(), it looks like __builtin_unreachable(). Not sure I see a nice solution, though. I'd advocate for a vkd3d_assert(), but there's no nice way to mark that function as "might not return".

  • Elizabeth Figura changed this line in version 3 of the diff

    changed this line in version 3 of the diff

  • Please register or sign in to reply
  • Francisco Casas approved this merge request

    approved this merge request

  • Francisco Casas
  • Francisco Casas
    Francisco Casas @fcasas started a thread on commit b885572c
  • 2182 2182 free_function(RB_ENTRY_VALUE(entry, struct hlsl_ir_function, entry));
    2183 2183 }
    2184 2184
    2185 2185 void hlsl_add_function(struct hlsl_ctx *ctx, char *name, struct hlsl_ir_function_decl *decl, bool intrinsic)
  • Giovanni Mascellani
  • Giovanni Mascellani
  • Elizabeth Figura added 5 commits

    added 5 commits

    • cc8e965c - tests: Test specifying a UAV address as an in/out parameter to a function.
    • c7fbfba2 - vkd3d-shader/hlsl: Avoid assuming that expressions have at least one argument.
    • 0b72d595 - vkd3d-shader/hlsl: Generate IR for user-defined function calls.
    • a63e534f - vkd3d-shader/hlsl: Forbid returning void expressions from void functions.
    • d486fe47 - vkd3d-shader/hlsl: Get rid of the "intrinsic" field of struct hlsl_ir_function.

    Compare with previous version

  • Elizabeth Figura added 3 commits

    added 3 commits

    • 5e1b5e9e - vkd3d-shader/hlsl: Generate IR for user-defined function calls.
    • a130df9e - vkd3d-shader/hlsl: Forbid returning void expressions from void functions.
    • 7828ba6c - vkd3d-shader/hlsl: Get rid of the "intrinsic" field of struct hlsl_ir_function.

    Compare with previous version

  • Looks like this conflicts with !50 (merged), so this will need to be rebased after that one is committed.

  • Henri Verbeet mentioned in merge request !54 (merged)

    mentioned in merge request !54 (merged)

  • Elizabeth Figura added 36 commits

    added 36 commits

    • 7828ba6c...3fbe2726 - 31 commits from branch wine:master
    • 55696277 - tests: Test specifying a UAV address as an in/out parameter to a function.
    • f1fbff6a - vkd3d-shader/hlsl: Avoid assuming that expressions have at least one argument.
    • b59c18c7 - vkd3d-shader/hlsl: Generate IR for user-defined function calls.
    • d48ddf99 - vkd3d-shader/hlsl: Forbid returning void expressions from void functions.
    • 2c3f8343 - vkd3d-shader/hlsl: Get rid of the "intrinsic" field of struct hlsl_ir_function.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading