Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Fold logic AND and logic OR identities.

Merged Victor Chiletto requested to merge vitorhnn/vkd3d:logic-ops-identities into master

Merge request reports

Merge request pipeline #31266 skipped

Merge request pipeline skipped for a39227c7

Approved by

Merged by Henri VerbeetHenri Verbeet 7 months ago (Sep 3, 2024 3:17pm UTC)

Merge details

  • Changes merged into master with a39227c7.
  • Deleted the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Elizabeth Figura approved this merge request

    approved this merge request

  • @@ -1452,11 +1452,15 @@ static bool constant_is_one(struct hlsl_ir_constant *const_arg)
     
                 case HLSL_TYPE_UINT:
                 case HLSL_TYPE_INT:
    -            case HLSL_TYPE_BOOL:
                     if (const_arg->value.u[k].u != 1)
                         return false;
                     break;
     
    +            case HLSL_TYPE_BOOL:
    +                if (const_arg->value.u[k].u != ~0)
    +                    return false;
    +                break;
    +
                 default:
                     return false;
             }

    Is that right? My understanding was that any non-zero BOOL should evaluate to true/1.

    +        case HLSL_OP2_LOGIC_AND:
    +            if (constant_is_zero(const_arg))
    +                res_node = &const_arg->node;
    +            else if (constant_is_one(const_arg))
    +                res_node = mut_arg;
    +            break;
    +
    +        case HLSL_OP2_LOGIC_OR:
    +            if (constant_is_zero(const_arg))
    +                res_node = mut_arg;
    +            else if (constant_is_one(const_arg))
    +                res_node = &const_arg->node;
    +            break;

    Somewhat relatedly, are constant_is_zero() and constant_is_one() the right thing to use here, or should we introduce and use constant_is_false() and constant_is_true()?

    Also, is that really how these operators work in HLSL? In C, "x && 1" and "x || 0" evaluate to "!!x", not "x". Do we have test coverage for this? The closest I was able to find was tests/hlsl/vertex-shader-ops.shader_test, which seems to suggest this should behave like the C equivalents.

  • Is that right? My understanding was that any non-zero BOOL should evaluate to true/1.

    We always set them to ~0u in the IR (or at least, we should.) I feel a little meh about this decision, if only because it's more fragile than "nonzero is true", but I can't say it's wrong at this point.

    Also, is that really how these operators work in HLSL? In C, "x && 1" and "x || 0" evaluate to "!!x", not "x". Do we have test coverage for this? The closest I was able to find was tests/hlsl/vertex-shader-ops.shader_test, which seems to suggest this should behave like the C equivalents.

    We cast arguments to bool before adding the LOGIC_* expr, so from an IR perspective it already is "(!!x) && true".

  • Is that right? My understanding was that any non-zero BOOL should evaluate to true/1.

    We always set them to ~0u in the IR (or at least, we should.) I feel a little meh about this decision, if only because it's more fragile than "nonzero is true", but I can't say it's wrong at this point.

    Sure, but restricting constant_is_one() to ~0u seems like it mostly just makes things slightly more fragile with little upside. Anyway, I wasn't too worried about this bit.

    Also, is that really how these operators work in HLSL? In C, "x && 1" and "x || 0" evaluate to "!!x", not "x". Do we have test coverage for this? The closest I was able to find was tests/hlsl/vertex-shader-ops.shader_test, which seems to suggest this should behave like the C equivalents.

    We cast arguments to bool before adding the LOGIC_* expr, so from an IR perspective it already is "(!!x) && true".

    Right, makes sense.

  • Henri Verbeet approved this merge request

    approved this merge request

  • Henri Verbeet added 19 commits

    added 19 commits

    Compare with previous version

Please register or sign in to reply
Loading