Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Improve constant handling in copy prop, and extend the use of aoffimmi modifiers.

Merged Francisco Casas requested to merge fcasas/vkd3d:aoffimmi into master

If a hlsl_ir_load loads a variable whose components are stored from different instructions, copy propagation doesn't replace it.

But if all these instructions are constants (which currently is the case for value constructors), the load can be replaced with a constant value, which is what the first patch of this series does.

For instance, this shader:

sampler s;
Texture2D t;

float4 main() : sv_target
{
    return t.Gather(s, float2(0.6, 0.6), int2(0, 0));
}

results in the following IR before applying the patch:

  float | 6.00000024e-01
  float | 6.00000024e-01
   uint | 0
        | = (<constructor-2>[@4].x @2)
   uint | 1
        | = (<constructor-2>[@6].x @3)
 float2 | <constructor-2>
    int | 0
    int | 0
   uint | 0
        | = (<constructor-5>[@11].x @9)
   uint | 1
        | = (<constructor-5>[@13].x @10)
   int2 | <constructor-5>
 float4 | gather_red(resource = t, sampler = s, coords = @8, offset = @15)
        | return
        | = (<output-sv_target0> @16)

and this IR afterwards:

 float2 | {6.00000024e-01 6.00000024e-01 }
   int2 | {0 0 }
 float4 | gather_red(resource = t, sampler = s, coords = @2, offset = @3)
        | return
        | = (<output-sv_target0> @4)

This is required to write texel_offsets as aoffimmi modifiers in the sm4 backend, since it expects the texel_offset arguments to be hlsl_ir_constant.

This series also:

  • Allows Gather() methods to use aoffimmi modifiers instead of an additional source register (which is the only way allowed for shader model 4.1), when possible.
  • Adds support to texel_offsets in the Load() method via aoffimmi modifiers (the only allowed method).

Merge request reports

Merge request pipeline #6699 skipped

Merge request pipeline skipped for 6b82ba94

Merged by Alexandre JulliardAlexandre Julliard 2 years ago (Jan 24, 2023 9:27pm 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
    • Giovanni actually sent a very similar patch to 1/4 [1] several months ago. It wasn't accepted, and I'm not sure what discussion there was about it if any (it may have been off-list).

      I think that this is, broadly speaking, not a wrong solution—and if we care about getting texel offsets working I don't mind accepting it—but I don't think it's quite the right one either. I think there are two other passes that we can do instead that would do everything this pass does and more:

      (1) generic vectorization of stores (2) propagation of load+store sequences into multiple store instructions

      Consider specifically the following shader:

      uniform float f;
      
      float4 main() : sv_target
      {
          return float4(f, 1, 2, 3);
      }

      which generates:

       2:      float | f
       3:       uint | 0 
       4:            | = (<constructor-2>[@3].x @2)
       5:      float | 1.00000000e+00 
       6:       uint | 1 
       7:            | = (<constructor-2>[@6].x @5)
       8:      float | 2.00000000e+00 
       9:       uint | 2 
      10:            | = (<constructor-2>[@9].x @8)
      11:      float | 3.00000000e+00 
      12:       uint | 3 
      13:            | = (<constructor-2>[@12].x @11)
      14:     float4 | <constructor-2>
      15:            | return
      16:            | = (<output-sv_target0> @14)

      Copyprop-with-constant-loads won't help this. However, with the first pass I describe, we can simplify that into:

       2:      float | f
       3:       uint | 0 
       4:            | = (<constructor-2>[@3].x @2)
       5:     float3 | {1.0 2.0 3.0}
       6:       uint | 1 
       7:            | = (<constructor-2>[@6].xyz @5)
      14:     float4 | <constructor-2>
      15:            | return
      16:            | = (<output-sv_target0> @14)

      The second pass would recognize when the rhs of a store instruction is just a load, and rewrite it as multiple store instructions. (These passes could be applied in either order, fwiw):

       2:      float | f
       3:       uint | 0 
       4:            | = (<output-sv_target0>[@3].x @2)
       5:     float3 | {1.0 2.0 3.0}
       6:       uint | 1 
       7:            | = (<output-sv_target0>[@6].xyz @5)

      (Ignoring the return statement for now. Also, I've automatically applied DCE to these examples.)

      Note that this second pass doesn't necessarily have to be limited to whole-variable stores either. (Note also that we can't do the same thing with copy-prop because a load instruction has to return the whole vector type; it can't be split.)

      This doesn't fully replace copy-prop as it is, to be sure, but I think it replaces everything that patch 1/4 does. Copy-prop is designed to replace X-store-load sequences with X, to oversimplify. If vectorization can turn X into a single instruction, then copy-prop can get rid of the store/load and probably the whole temp variable. If vectorization can't do that, then we needed that temp anyway.

      [1] https://www.winehq.org/pipermail/wine-devel/2022-May/215773.html

    • I think I described this second pass poorly. It's actually probably easier to think of it as an extension of copy-prop.

      The idea is, if you have something like that float4(f, 1, 2, 3) sequence, you can't copy-prop loads from <constructor-2> with the current pass because they don't all have the same instruction as a source. So what you do instead is, any time that <constructor-2> is itself used as an rhs to a store, you split up that store into multiple stores. You could say one per unique source but you could also do it more simply by saying one per component (and then letting vectorization clean that up later). That way you're guaranteed to be able to copy-prop them. You increase the number of stores but (hopefully) reduce the number of intermediate variables.

      Actually, now that I describe it this way, I realize it doesn't even have to be limited to stores, it can be anything that's done per-component, so you could do a similar thing with some HLSL_IR_EXPRs.

    • Actually, now that I describe it this way, I realize it doesn't even have to be limited to stores, it can be anything that's done per-component, so you could do a similar thing with some HLSL_IR_EXPRs.

      Eh, except that the thing that's unique about stores is that you can "just" split them, because they don't produce a result. Expressions do. So ignore that last bit :-)

    • The idea is, if you have something like that float4(f, 1, 2, 3) sequence, you can't copy-prop loads from <constructor-2> with the current pass because they don't all have the same instruction as a source. So what you do instead is, any time that <constructor-2> is itself used as an rhs to a store, you split up that store into multiple stores. You could say one per unique source but you could also do it more simply by saying one per component (and then letting vectorization clean that up later). That way you're guaranteed to be able to copy-prop them. You increase the number of stores but (hopefully) reduce the number of intermediate variables.

      My way to read your proposal (for the second step) is as a sort of variable deduplication (or maybe dealiasing?). What's happening in your test program is that <constructor-2> and <output-sv_target0> are essentially the same variable: as soon as they both are initialized they have the same value, and they will keep it for their whole lifespan. So you can basically replace one with the other one. In theory you can do it either way, but in this case you need <output-sv_target0> to survive because it has an externally visible semantic.

      Edited by Giovanni Mascellani
    • Giovanni actually sent a very similar patch to 1/4 [1] several months ago. It wasn't accepted, and I'm not sure what discussion there was about it if any (it may have been off-list).

      The reason I remember was more or less the same thing you say now: there are more generic passes that can be written instead of this trick, and in practice it didn't really enable anything relevant, so you proposed to rather wait for somebody to write the proper thing. Since now it could enable the aoffimmi modifiers there could be a better reason to have it immediately (or, rather, after the freeze), even if it's not the best possible solution yet.

      Though I am not sure of what you mean by "generic vectorization of stores". The peculiar feature that is exploited by this patch is the fact that immediate constants can be vectorized, i.e., you can replace

      a.x = 1;
      a.y = 2;
      a.z = 3;

      with

      a.xyz = {1, 2, 3};

      But as I said that's rather a property of immediate constants, rather than of stores. In other words, I would describe a "generic vectorization of stores" as something that replaces

      a.x = b.x;
      a.y = b.y;
      a.z = b.z;

      with

      a = b;

      That's not something you can do in Francisco's example unless you know that immediate constants are special, i.e., you can vectorize them even if they come from different sources. Which is basically what my patch was doing (and what I believe this patch does; I say "believe" because I haven't read it yet in detail). So it seems to me that in one way or another you always need to have something like this patch, even if it would be good to also handle the cases in which only some components are immediate constants.

      Or maybe you had already disproved the thing I just wrote and I don't remember any more? I wouldn't be unheard of for my brain to randomly drop pieces of information.

    • The idea is, if you have something like that float4(f, 1, 2, 3) sequence, you can't copy-prop loads from <constructor-2> with the current pass because they don't all have the same instruction as a source. So what you do instead is, any time that <constructor-2> is itself used as an rhs to a store, you split up that store into multiple stores. You could say one per unique source but you could also do it more simply by saying one per component (and then letting vectorization clean that up later). That way you're guaranteed to be able to copy-prop them. You increase the number of stores but (hopefully) reduce the number of intermediate variables.

      My way to read your proposal (for the second step) is as a sort of variable deduplication (or maybe dealiasing?). What's happening in your test program is that <constructor-2> and <output-sv_target0> are essentially the same variable: as soon as they both are initialized they have the same value, and they will keep it for their whole lifespan. So you can basically replace one with the other one. In theory you can do it either way, but in this case you need <output-sv_target0> to survive because it has an externally visible semantic.

      Kind of, but it's more general than that. In theory the "intermediate" variable (in this case the constructor) can be used elsewhere, and it doesn't have to have the same component count as the "final" one. Although in practice it'll probably just be useful for the more restricted case of equal and "duplicate" variables.

      Though I am not sure of what you mean by "generic vectorization of stores". The peculiar feature that is exploited by this patch is the fact that immediate constants can be vectorized, i.e., you can replace

      a.x = 1;
      a.y = 2;
      a.z = 3;

      with

      a.xyz = {1, 2, 3};

      But as I said that's rather a property of immediate constants, rather than of stores. In other words, I would describe a "generic vectorization of stores" as something that replaces

      a.x = b.x;
      a.y = b.y;
      a.z = b.z;

      with

      a = b;

      That's not something you can do in Francisco's example unless you know that immediate constants are special, i.e., you can vectorize them even if they come from different sources. Which is basically what my patch was doing (and what I believe this patch does; I say "believe" because I haven't read it yet in detail). So it seems to me that in one way or another you always need to have something like this patch, even if it would be good to also handle the cases in which only some components are immediate constants.

      No, I think language is just hard :D

      By "vectorization of stores" I meant the store to a in this example. I suppose it's redundant since it's not like it's really possible to vectorize anything else.

      The pass I describe is kind of specific to constants, although there are other vectorization passes you can do that might end up reusing the same infrastructure. E.g. in your second example you can vectorize loads from the same variable. There's also a possibility of vectorizing stores from the same node by creating a broadcast swizzle.

    • Author Developer

      Ok, so, in summary, I identify the following relevant passes:

      • a) Constant handling in copy-prop (basically, what this first patch and Giovanni's original implementation do).
      • b) vectorization of stores with rhs constants.
      • c.1) vectorization of stores with rhs loads from the same vector within a variable.
      • c.2) alternatively, vectorization of stores with rhs loads from the same register.
      • d.1) split of stores with rhs loads in copy-prop, grouping the components according to the instruction node of their copy_propagation_value.
      • d.2) alternatively, split of stores with rhs loads componet-wise (it seems that this doesn't need to be during copy-prop).

      I decided to separate (c) into (c.1) and (c.2) because the latter would be harder to implement with the current architecture but have more reach. Also, it may be more convenient to do it after register allocation.

      I think Zeb's "generic vectorization of stores" maps to (b), although before her last message I thought she meant a pass that does both (b) and (c) because it had "generic" in the name. Probably Gio thought the same.

      Also, I think both (d.1) and (d.2) map to Zeb's "propagation of load+store sequences into multiple store instructions" proposals.

      So, the idea is to achieve all the functionality of (a) and also achieve additional vectorization through the implementation of just (b) and (d).

      I will start writing my thoughts on these alternatives.

      Edited by Francisco Casas
    • Author Developer

      Regarding (b) and (c), i.e. vectorization:

      To achieve vectorization, in particular (c) for, say:

      a.x = b.x;
      // other instructions
      a.y = b.y

      If we merge the two operations down, we have to make sure that:

      • a.x is not read/write by the other instructions in between.
      • b.x is not written in between.

      If we merge the two operations up, we have to make sure that:

      • a.y is not read/write by the other instructions in between.
      • b.y is not written in between, and its hlsl_ir_load is before the a.x = instruction, or can be moved there.

      Were we also have to consider the possibility of non-constant paths accesing these values.

      (b) would be easier to achieve than (c) since, if we replace b.x and b.y with constants in this example, we know that these values will never be written to. Also, (c) is more complex since we also have to keep an eye on the location of the b.x and b.y hlsl_ir_load·s in the IR.

      Since this pass wouldn't be helping copy-prop, it probably makes sense to run it after the do ... while(progress) that includes copy-prop and friends.

      To implement this, we can write a function to check whether is possible for a value to be accessed for read/write within a list of instructions, giving an starting point and an end point, and use it to check if these conditions meet for each pair of instructions that are compatible (both are stores from b to a in different components).

      However, to have the same reach as copy-prop, we would also have to consider how to handle control flow with this pass. There may be ifs or loops in between, or one of the instructions may be inside a block where the other is not. Since I don't have clear answers on how to implement the latter cases, I assume (b) and (c) would only operate for pairs of instructions in the same block.

    • You probably want to split, at least at the conceptual level, this into two different steps: first, what optimization can you do when the relevant instructions are already consecutive (i.e., your other instructions block is empty); second, how to commute instructions in order to make "interesting" instruction consecutive. You're probably going to reuse the second step for many different passes (again, at the conceptual level; it's not obvious that code will be easy to reuse).

      So, when can you commute two IR instructions? And when can you move an IR instruction outside or inside a block? Clearly you always have to keep things consequential, so you can't move x after y if y uses the result of x. Then things become a bit more complicated and I'm doing little more than brainstorming, so take this with a grain of salt. Maybe two or three grains.

      • Constants, expressions and swizzles commute happily with more or less everything else, and can go inside or outside code blocks.
      • Loads and stores only touch thread-local stuff (I am not sure that "thread" is the right word in the GPU world, but I guess you get what I mean), so commute among themselves (provided they don't touch the same registers) and with resource loads and stores. They don't commute (in general! They do in some cases) with blocks and jumps, or cannot go inside or outside code blocks.
      • Resource loads and stores are more interesting, because they interact with other threads. The way they commute is dictated by the memory model. Searching the web for "hlsl memory model" or "sm4 memory model" gives basically no useful result, but I guess we can assume that it will be rather weak, so resource loads and stores can commute rather easily, except when they touch the same address. Or maybe even in that case, who knows? The more I discover about GPUs the more I am baffled. At some point we'll bring in barriers to prevent them commuting too much, but for the moment we don't support them. If in doubt, it's more appropriate to err towards a stronger memory model (allowing fewer optimizations, but avoiding introducing miscompilations).

      As usual, lots of fun to be had!

    • Please register or sign in to reply
  • Author Developer

    Regarding (c.1) vs (c.2)

    We have the following dilemma:

    We can choose between vectorizing at the vector level, or try to do it at the register level. I think the difference only becomes apparent when more than one vector/scalar share the same register.

    On one hand, vectorizing register-wise is more general and is a better optimization. On the other, it is more complicated as we either should be considering register offsets at the IR level when running this passes, identifying in the struct if two components share the same register, or we should be deferring the pass after register allocation.

    So far, I don't think we need (c) for some reason other than optimizing the output code and making the IR more compact, but it is worth thinking about it.

    Actually, (b) is also not exempt of this dilemma, since rhs could also vectorized vector-wise or register-wise. (maybe we should call them (b.1) and (b.2)).

    Edited by Francisco Casas
  • Author Developer

    Regarding (d.1) vs (d.2)

    I suspect that (d.1) has not much value over (d.2), since both will split more stores than necessary and that can only be repaired with (c). The only thing is that (d.1) may generate less of them.

    If we put (d.1) before or inside the do ... while(progress) (or as an extension of copy-prop, which makes sense), it may be splitting with more granularity than necessary, since, revealing that values come from the same instruction may take several of these iterations, and if we place it after, it won't help copy prop.

    (d.2) can be put before the do ... while(progress) and executed once.

  • Author Developer

    Regarding (b) and (d) vs. (a)

    In my opinion, this reduces to two questions:

    1. (b) and (d) indeed cover all the cases that (a) does?
    2. What cases do (b) and (d) cover that (a) doesn't?

    Surprisingly, now I think that the answer to question (1) is no. Consider the following example which compiles in native and with pass (a):

    Texture2D t;
    uniform int i;
    
    float4 main() : sv_target
    {
        int2 a = {1, 0};
    
        if (i < 5)
        {
            a.y = 3;
            return t.Load(int3(0, 1, 2), a);
        }
        else
        {
            a.y = 4;
            return t.Load(int3(0, 1, 2), a);
        }
    }

    in this case two constant vector2s are to be generated, but as they share a common constant component, the vectorization of the constants alone wouldn't be able to duplicate this component. I didn't put more thought on it, but there may also be more rudimentary examples.

    Regarding question (2), at first I thought that the benefit was just a matter of code readability, which alone wouldn't be achieved unless we also implemented (c). However, then I found the following case that (b) and (d) would cover that (a) won't, see the following message.

  • Author Developer

    The swizzle issue

    (a) in its current form is not addressing swizzles.

    Consider the following shader:

    Texture2D t;
    uniform int i;
    
    float4 main() : sv_target
    {
        int3 a = {1, 2, i};
        return t.Load(int3(0, 1, 2), a.xy);
    }

    even with (a), this results in:

       2:        int | i
       3:        int | 1 
       4:       uint | 0 
       5:            | = (a[@4].x @3)
       6:        int | 2 
       7:       uint | 1 
       8:            | = (a[@7].x @6)
       9:       uint | 2 
      10:            | = (a[@9].x @2)
      11:       int3 | {0 1 2 }
      12:       int3 | a
      13:       int2 | @12.xy
      14:     float4 | load_resource(resource = t, sampler = (nil), coords = @11, offset = @13)
      15:            | return
      16:            | = (<output-sv_target0> @14)

    and the offset cannot be resolved as a constant int2.

    This may be a good reason to also include (b) + (d)

    Edited by Francisco Casas
  • Author Developer

    Conclusions

    I hope my analysis was useful but there are too many things in play and some of my assessments may be wrong, so be aware that there may be errors and point them if you find them! I will also keep thinking to see if I find them or counter examples that change my opinion, there is still a lot to think here.

    Having said that, my current opinion is that we should implement (a) + (b.1) + (d.2), but also seek for alternatives that just involve (a) and some way of solving the swizzle problems (if we can make sure that they cover what (b) + (d) does), like the mentioned lowering of swizzles as component-wise stores.

    Since I don't think that IR code readability alone has priority over functionality, and AFAIK we can trust the drivers to do vectorization for us, I also think we should defer (c) unless we find an actual feature that requires it.

    There is also the alternative of using hlsl_deref·s instead of mere nodes as texel_offsets, but IIRC Zeb has good reasons to not go in that direction.

    Edited by Francisco Casas
  • We can choose between vectorizing at the vector level, or try to do it at the register level. I think the difference only becomes apparent when more than one vector/scalar share the same register.

    On one hand, vectorizing register-wise is more general and is a better optimization. On the other, it is more complicated as we either should be considering register offsets at the IR level when running this passes, identifying in the struct if two components share the same register, or we should be deferring the pass after register allocation.

    This is a difficult question, actually. I think the options are

    (i) vectorize as much as possible, most notably past 4 components, and let translation into smX split up the ops again. This isn't bad conceptually, but the current codegen is really built to not do this.

    (ii) vectorize up to 4 components and along aligned boundaries. The problem is that this is leaking backend details out of the backends again.

    (iii) don't vectorize at all and make the backends do it instead. Disadvantage is that we may then need to code it multiple times.

    There are some passes we'll need to do per-backend anyway, e.g. getting rid of the redundant movs from HLSL_IR_LOAD instructions. I'm not sure if this is such a case?

    So far, I don't think we need (c) for some reason other than optimizing the output code and making the IR more compact, but it is worth thinking about it.

    I'm inclined to agree, yeah. If nothing else it hasn't been necessary for anything, it was just a vague idea of something we could extend the pass too eventually.

    Actually, (b) is also not exempt of this dilemma, since rhs could also vectorized vector-wise or register-wise. (maybe we should call them (b.1) and (b.2)).

    (b) is about vectorizing constants, though? Probably we'd only vectorize them up to 4. Granted, though, you get the same issues as above, and vectorizing constants for sm1 also involves some tradeoffs if we leave that in common code.

    1. (b) and (d) indeed cover all the cases that (a) does?
    2. What cases do (b) and (d) cover that (a) doesn't?

    Surprisingly, now I think that the answer to question (1) is no. Consider the following example which compiles in native and with pass (a):

    Hmm, true. The alternate way to solve that is to pull that variable into the loop, but that requires knowing when the variable should be pulled into the loop (in general we probably want to go the other direction), and I can't easily think of another reason we'd want to do that (to alleviate register pressure, maybe?)

    So (a) probably does make the most sense after all.

    There is also the alternative of using hlsl_deref·s instead of mere nodes as texel_offsets, but IIRC Zeb has good reasons to not go in that direction.

    I don't remember this ever being proposed? I don't see any benefit to it, either.

    That we use hlsl_deref instead of hlsl_src for resource/sampler arguments to hlsl_ir_resource_load is... odd. Odd because every other argument expression is hlsl_src, and there's no obvious reason that these would be different. It works, though, because object types can always be resolved to a deref (i.e. if they were hlsl_src, they'd be guaranteed to come from HLSL_IR_LOAD), and it's necessary because we can't translate HLSL_IR_LOAD to an IR instruction.

  • added 1 commit

    • 0121ac38 - vkd3d-shader/hlsl: Replace swizzles with constants in copy prop.

    Compare with previous version

  • Author Developer

    Huh, sorry. This is not yet complete, I pushed by mistake.

    But my plan is to keep (a) and also support replacing swizzles with constants if the required components can be resolved into a constant, which I think should be enough for this MR.

    This last patch does that, but I should also implement a way to combine chained swizzles:

    uniform uint i;
    
    float4 main() : sv_target
    {
        float4 a = {1, 2, 3, i};
        float4 b = a.xywz;
        float4 c = b.yxww;
        return c; // Should be constant (2, 1, 3, 3)
    }
  • Francisco Casas added 4 commits

    added 4 commits

    • 7082e49d - vkd3d-shader/hlsl: Support offset argument for the texture Load() method.
    • 67260b96 - tests: Test constant propagation through swizzles.
    • 9c6e0ef0 - vkd3d-shader/hlsl: Replace swizzles with constants in copy prop.
    • 2115bdee - vkd3d-shader/hlsl: Propagate swizzle chains in copy propagation.

    Compare with previous version

  • Author Developer

    Rebasing...

  • Francisco Casas added 13 commits

    added 13 commits

    • 2115bdee...1b11b576 - 6 commits from branch wine:master
    • 0af57def - vkd3d-shader/hlsl: Replace loads with constants in copy prop.
    • b39f0ffb - vkd3d-shader/hlsl: Use aoffimmis when writing gather methods.
    • 59ac942e - tests: Test offset argument for the texture Load() method.
    • fe8e7a74 - vkd3d-shader/hlsl: Support offset argument for the texture Load() method.
    • 22e6fd6e - tests: Test constant propagation through swizzles.
    • f4dba11f - vkd3d-shader/hlsl: Replace swizzles with constants in copy prop.
    • 6b02aeaf - vkd3d-shader/hlsl: Propagate swizzle chains in copy propagation.

    Compare with previous version

    • Wrt patch 7/7, why not just add a pass that combines multiple swizzles outside of copy-prop?

    • Author Developer

      Hmm, good point, this could indeed be separated from copy-prop, as part of the while(progress) loop.

      I think that this process is very related to copy-prop though, since, strictly speaking, it doesn't combine swizzles, but instead makes swizzle nodes that reference other swizzles to directly reference the source of the value.

      e.g. if we have multiple swizzles:

      b = a.xz;
      c = b.yy;

      what this does is keeping the same number of swizzles but it lowers 'c' so that it directly references the first non-swizzle node (a in this case):

      b = a.xz;
      c = a.zz;

      I think swizzle_propagation could be a good name for this pass, but I am not totally sure.

      Edited by Francisco Casas
    • Sure, but a separate pass would also deal with "a.xz.yy". Granted, probably nobody would ever write that intentionally (well, maybe with macros it's plausible, though?)

    • Author Developer

      Okay, writing it as a separated pass!

      I think the current version also deals with swizzles like "a.xz.yy" btw. Gotta add tests for those too.

    • Please register or sign in to reply
  • Francisco Casas
    Francisco Casas @fcasas started a thread on an outdated change in commit f4dba11f
995 1059 copy_propagation_record_store(ctx, hlsl_ir_store(instr), state);
996 1060 break;
997 1061
1062 case HLSL_IR_SWIZZLE:
1063 copy_propagation_transform_swizzle(ctx, hlsl_ir_swizzle(instr), state);
  • Francisco Casas added 3 commits

    added 3 commits

    • 9735b1d0 - tests: Test constant propagation through swizzles.
    • 0860586b - vkd3d-shader/hlsl: Replace swizzles with constants in copy prop.
    • 90b00153 - vkd3d-shader/hlsl: Propagate swizzle chains.

    Compare with previous version

  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 90b00153
  • 1351 1351 }
    1352 1352
    1353 static bool propagate_swizzle_chains(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context)
    1354 {
    1355 struct hlsl_ir_swizzle *swizzle;
    1356 struct hlsl_ir_node *next_val;
    1357 unsigned int combined_swizzle;
    1358
    1359 if (instr->type != HLSL_IR_SWIZZLE)
    1360 return false;
    1361 swizzle = hlsl_ir_swizzle(instr);
    1362
    1363 next_val = swizzle->val.node;
    1364 combined_swizzle = swizzle->swizzle;
    1365
    1366 while (next_val->type == HLSL_IR_SWIZZLE)
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 90b00153
  • 1350 1350 return false;
    1351 1351 }
    1352 1352
    1353 static bool propagate_swizzle_chains(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context)
    • Giving a closer look to patch 6/7, it doesn't quite look like I'd expect. We should be able to handle load-store-load-swizzle as well as constant-store-load-swizzle. I'd expect that we should be passing a (unsigned int) swizzle parameter to copy_propagation_transform_load() and its inferiors, which would be HLSL_SWIZZLE(X, Y, Z, W) for the current user.

      For that matter, we could probably factor out creating an array of sources, e.g.

      struct copy_propagation_value *values[4];
      
      for (...)
         values[i] = copy_propagation_get_values(state, values, var, start + swizzled_component(swizzle, i));

      and then pass that array to copy_propagation_compute_*_replacement().

    • Author Developer

      Good point, if I understood the problem correctly, the new approach covers this.

    • That's not quite what I'm thinking of either, and it looks more than a little awkward now. The point is (a) we shouldn't have to specially handle loads and swizzles in copy_propagation_transform_swizzle(), and (b) now copy_propagation_compute_constant_replacement() is a weird anti-helper that cares about the type of its argument.

      Because this is copy-prop, we are only dealing with swizzled loads, not just any swizzles. So copy_propagation_transform_swizzle() should probably contain that check explicitly. And then my idea is, I think, something like this:

      static bool copy_propagation_transform_load(struct hlsl_ctx *ctx,
              struct hlsl_ir_load *load, struct copy_propagation_state *state, unsigned int swizzle)
      {
          ...
      }
      
      static bool copy_propagation_transform_swizzle(...)
      {
          ...
      
          if (swizzle->val->type != HLSL_IR_LOAD)
              return false;
      
          return copy_propagation_transform_load(ctx, hlsl_ir_load(swizzle->val), state, swizzle->swizzle);
      }
      
      static bool copy_propagation_transform_block(...)
      {
          ...
      
                  case HLSL_IR_LOAD:
                      progress |= copy_propagation_transform_load(ctx, hlsl_ir_load(instr), state, HLSL_SWIZZLE(X, Y, Z, W));
                      break;
          ...
      }
    • Author Developer

      Got it.

      I am afraid that copy_propagation_compute_replacement() will still require to receive an input swizzle argument though (which I think adds to the awkwardness). I will see if I can use the current output argument unsigned int *swizzle for both the input and output swizzles.

    • Author Developer

      Oh, I remember why I made copy_propagation_compute_load_constant_replacement() receive the whole instruction as an argument. It is that the swizzle alone doesn't convey number of components requested, and that also isn't in the load itself, but rather in the swizzle instruction. I should add it as an additional argument then.

      Edited by Francisco Casas
    • Author Developer

      Actually, this doesn't make much sense:

      static bool copy_propagation_transform_swizzle(...)
      {
          ...
      
          if (swizzle->val->type != HLSL_IR_LOAD)
              return false;
      
          return copy_propagation_transform_load(ctx, hlsl_ir_load(swizzle->val), state, swizzle->swizzle);
      }

      because calling copy_propagation_transform_load would be replacing the swizzle's load, and not the swizzle itself. So I'm going to skip that part of the advice and see where I get to.

    • I am afraid that copy_propagation_compute_replacement() will still require to receive an input swizzle argument though (which I think adds to the awkwardness). I will see if I can use the current output argument unsigned int *swizzle for both the input and output swizzles.

      Yep, that sounds right. I don't have a strong preference at the moment between using an in/out argument or using two separate arguments.

      Oh, I remember why I made copy_propagation_compute_load_constant_replacement() receive the whole instruction as an argument. It is that the swizzle alone doesn't convey number of components requested, and that also isn't in the load itself, but rather in the swizzle instruction. I should add it as an additional argument then.

      Yes, a separate component count makes sense to me.

      Actually, this doesn't make much sense:

      static bool copy_propagation_transform_swizzle(...)
      {
          ...
      
          if (swizzle->val->type != HLSL_IR_LOAD)
              return false;
      
          return copy_propagation_transform_load(ctx, hlsl_ir_load(swizzle->val), state, swizzle->swizzle);
      }

      because calling copy_propagation_transform_load would be replacing the swizzle's load, and not the swizzle itself. So I'm going to skip that part of the advice and see where I get to.

      Eh, right, you'd need to pull out the hlsl_replace_node() part. But the general idea should still be similar; we want most of that code to be shared.

    • Please register or sign in to reply
  • Francisco Casas added 3 commits

    added 3 commits

    • ae8e785c - vkd3d-shader/hlsl: Replace swizzles with constants in copy prop.
    • d3972734 - vkd3d-shader/hlsl: Propagate copies through swizzles.
    • 1fff9d3f - vkd3d-shader/hlsl: Fold swizzle chains.

    Compare with previous version

  • Francisco Casas added 4 commits

    added 4 commits

    • 653b788a - vkd3d-shader/hlsl: Replace swizzles with constants in copy prop.
    • 999c174b - vkd3d-shader/hlsl: Propagate copies through swizzles.
    • db4e2d84 - vkd3d-shader/hlsl: Refactor common code on copy-prop of both loads and swizzles.
    • 04386feb - vkd3d-shader/hlsl: Fold swizzle chains.

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading