[Mesa-dev] [PATCH] mesa/st/glsl_to_tgsi: Mark first write as unconditional when apropriate

[Brian Paul]

On 01/29/2018 05:24 AM, Gert Wollny wrote:
> In the register lifetime estimation if the first write is unconditional or
> conditional but not within a loop than this is an unconditional dominant

than -> then

> write in the sense of register life time estimation.
> Add a test case and record the write accordingly.

I don't really understand the code, but the patch looks OK to me.

Reviewed-by: Brian Paul <brianp at vmware.com>

I can push this in a bit.


> 
> Fixes: 807e2539e512ca6c96f059da855473eb7be99ba1
>    mesa/st/glsl_to_tgsi: Add tracking of ifelse writes in register merging
> Bugzilla: https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D104803&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=Ie7_encNUsqxbSRbqbNgofw0ITcfE8JKfaUjIQhncGA&m=HYtcsBqPlkbxRGKakM7-fRJbICIqO4cx61Ah07d_cUw&s=bp-ajSeskM47lfjWPMZYje1thrrlRao8t7EUbblmU3I&e=
> Signed-off-by: Gert Wollny <gw.fossdev at gmail.com>
> ---
>   .../state_tracker/st_glsl_to_tgsi_temprename.cpp   | 24 +++++++++++++++++++++-
>   .../tests/test_glsl_to_tgsi_lifetime.cpp           | 24 ++++++++++++++++++++++
>   2 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi_temprename.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi_temprename.cpp
> index 3a00b33749..6921a643b7 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi_temprename.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi_temprename.cpp
> @@ -197,6 +197,7 @@ private:
>      static const int write_is_conditional = -1;
>      static const int conditionality_unresolved = 0;
>      static const int conditionality_untouched;
> +   static const int write_is_unconditional;
>   
>      /* A bit field tracking the nexting levels of if-else clauses where the
>       * temporary has (so far) been written to in the if branch, but not in the
> @@ -220,6 +221,9 @@ private:
>   const int
>   temp_comp_access::conditionality_untouched = numeric_limits<int>::max();
>   
> +const int
> +temp_comp_access::write_is_unconditional = numeric_limits<int>::max() - 1;
> +
>   /* Class to track the access to all components of a temporary register. */
>   class temp_access {
>   public:
> @@ -566,6 +570,13 @@ void temp_comp_access::record_read(int line, prog_scope *scope)
>         first_read_scope = scope;
>      }
>   
> +   /* If the conditionality of the first write is already resolved then
> +    * no further checks are required.
> +    */
> +   if (conditionality_in_loop_id == write_is_unconditional ||
> +       conditionality_in_loop_id == write_is_conditional)
> +      return;
> +
>      /* Check whether we are in a condition within a loop */
>      const prog_scope *ifelse_scope = scope->in_ifelse_scope();
>      const prog_scope *enclosing_loop;
> @@ -612,9 +623,20 @@ void temp_comp_access::record_write(int line, prog_scope *scope)
>      if (first_write < 0) {
>         first_write = line;
>         first_write_scope = scope;
> +
> +      /* If the first write we encounter is not in a conditional branch, or
> +       * the conditional write is not within a loop, then this is to be
> +       * considered an unconditional dominant write.
> +       */
> +      const prog_scope *conditional = scope->enclosing_conditional();
> +      if (!conditional || !conditional->innermost_loop()) {
> +         conditionality_in_loop_id = write_is_unconditional;
> +      }
>      }
>   
> -   if (conditionality_in_loop_id == write_is_conditional)
> +   /* The conditionality of the first write is already resolved. */
> +   if (conditionality_in_loop_id == write_is_unconditional ||
> +       conditionality_in_loop_id == write_is_conditional)
>         return;
>   
>      /* If the nesting depth is larger than the supported level,
> diff --git a/src/mesa/state_tracker/tests/test_glsl_to_tgsi_lifetime.cpp b/src/mesa/state_tracker/tests/test_glsl_to_tgsi_lifetime.cpp
> index a3b0f0e02f..acebfb8293 100644
> --- a/src/mesa/state_tracker/tests/test_glsl_to_tgsi_lifetime.cpp
> +++ b/src/mesa/state_tracker/tests/test_glsl_to_tgsi_lifetime.cpp
> @@ -794,6 +794,30 @@ TEST_F(LifetimeEvaluatorExactTest, WriteInIfElseBranchSecondIfInLoop)
>      run (code, temp_lt_expect({{-1,-1}, {2,9}}));
>   }
>   
> +/** Regression test for bug #104803,
> + *  Read and write in if/else path outside loop and later read in conditional
> + *  within a loop. The first write is to be considered the dominant write.
> + */
> +TEST_F(LifetimeEvaluatorExactTest, IfElseWriteInBothOutsideLoopReadInElseInLoop)
> +{
> +   const vector<FakeCodeline> code = {
> +      { TGSI_OPCODE_IF, {}, {in0}, {} },
> +      {   TGSI_OPCODE_MOV, {1}, {in0}, {} },
> +      { TGSI_OPCODE_ELSE, {}, {}, {} },
> +      {   TGSI_OPCODE_MOV, {1}, {in1}, {} },
> +      { TGSI_OPCODE_ENDIF, {}, {}, {} },
> +      { TGSI_OPCODE_BGNLOOP },
> +      {   TGSI_OPCODE_IF, {}, {in0}, {} },
> +      {     TGSI_OPCODE_MOV, {2}, {in1}, {} },
> +      {   TGSI_OPCODE_ELSE, {}, {}, {} },
> +      {     TGSI_OPCODE_MOV, {2}, {1}, {} },
> +      {   TGSI_OPCODE_ENDIF, {}, {}, {} },
> +      { TGSI_OPCODE_ENDLOOP },
> +      { TGSI_OPCODE_MOV, {out0}, {2}, {}},
> +      { TGSI_OPCODE_END}
> +   };
> +   run (code, temp_lt_expect({{-1,-1}, {1,11}, {7, 12}}));
> +}
>   
>   /* A continue in the loop is not relevant */
>   TEST_F(LifetimeEvaluatorExactTest, LoopWithWriteAfterContinue)
>