subject:"\"\\\\\\\[Mesa\\\\\\\-dev\\\\\\\] \\\\\\\[PATCH 3\\\\\\\/3\\\\\\\] ac\\\\\\\: make use of if\\\\\\\/loop build helpers\""

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

2018-04-10 Thread Alex Smith

On 10 April 2018 at 15:49, Juan A. Suarez Romero 
wrote:

> On Tue, 2018-04-03 at 10:58 +0100, Alex Smith wrote:
> > I don't know exactly what's causing it, no. I noticed the issue was
> fixed on master so just bisected to this.
> >
> > CC'ing stable to nominate:
> > 42627dabb4db3011825a022325be7ae9b51103d6 - (1/3) ac: add if/loop build
> helpers
> > 6e1a142863b368a032e333f09feb107241446053 - (2/3) radeonsi: make use of
> if/loop build helpers in ac
> > 99cdc019bf6fe11c135b7544ef6daf4ac964fa24 - (3/3) ac: make use of
> if/loop build helpers
> >
>
> Hi, Alex.
>
> Are these 3 commits nominated for a specific stable branch? From the CC
> not sure
> if you want to nominate them for 17.3, 18.0 or both.
>

They work for me on both 18.0 and 17.3, so I think they can be nominated
for both.

Thanks,
Alex


>
>
> J.A.
>
> >
> >
> > On 3 April 2018 at 10:45, Timothy Arceri  wrote:
> > > I have no issue with these going in stable if they fix bugs. Ideally
> we should create a piglit test to catch this also but presumably you guys
> don't actually know the exact shader combination thats tripping things up?
> > >
> > >
> > > On 03/04/18 19:36, Samuel Pitoiset wrote:
> > > > This fixes a rendering issue with Wolfenstein 2 as well. A backport
> sounds reasonable to me.
> > > >
> > > > On 04/03/2018 11:33 AM, Alex Smith wrote:
> > > > > Hi Timothy,
> > > > >
> > > > > This patch fixes some rendering issues I see with RADV on SI.
> > > > >
> > > > > It doesn't sound like it was really intended to fix anything, so
> possibly it's masking some other issue, but would you object to nominating
> the series for stable? Applying it on the 18.0 branch fixes the issue there
> as well.
> > > > >
> > > > > Thanks,
> > > > > Alex
> > > > >
> > > > > On 7 March 2018 at 20:43, Marek Olšák  mar...@gmail.com>> wrote:
> > > > >
> > > > > For the series:
> > > > >
> > > > > Reviewed-by: Marek Olšák  > > > > >
> > > > >
> > > > > Marek
> > > > >
> > > > > On Tue, Mar 6, 2018 at 8:40 PM, Timothy Arceri
> > > > > mailto:tarc...@itsqueeze.com>> wrote:
> > > > >  > These helpers insert the basic block in the same order as
> they
> > > > >  > appear in NIR making it easier to follow LLVM IR dumps. The
> helpers
> > > > >  > also insert more useful labels onto the blocks.
> > > > >  >
> > > > >  > TGSI use the line number of the corresponding opcode in the
> TGSI
> > > > >  > dump as the label id, here we use the corresponding block
> index
> > > > >  > from NIR.
> > > > >  > ---
> > > > >  >  src/amd/common/ac_nir_to_llvm.c | 60
> > > > > +
> > > > >  >  1 file changed, 18 insertions(+), 42 deletions(-)
> > > > >  >
> > > > >  > diff --git a/src/amd/common/ac_nir_to_llvm.c
> > > > > b/src/amd/common/ac_nir_to_llvm.c
> > > > >  > index cda91fe8bf..dc463ed253 100644
> > > > >  > --- a/src/amd/common/ac_nir_to_llvm.c
> > > > >  > +++ b/src/amd/common/ac_nir_to_llvm.c
> > > > >  > @@ -5237,17 +5237,15 @@ static void visit_ssa_undef(struct
> > > > > ac_nir_context *ctx,
> > > > >  > _mesa_hash_table_insert(ctx->defs, &instr->def,
> undef);
> > > > >  >  }
> > > > >  >
> > > > >  > -static void visit_jump(struct ac_nir_context *ctx,
> > > > >  > +static void visit_jump(struct ac_llvm_context *ctx,
> > > > >  >const nir_jump_instr *instr)
> > > > >  >  {
> > > > >  > switch (instr->type) {
> > > > >  > case nir_jump_break:
> > > > >  > -   LLVMBuildBr(ctx->ac.builder,
> ctx->break_block);
> > > > >  > -   LLVMClearInsertionPosition(
> ctx->ac.builder);
> > > > >  > +   ac_build_break(ctx);
> > > > >  > break;
> > > > >  > case nir_jump_continue:
> > > > >  > -   LLVMBuildBr(ctx->ac.builder,
> ctx->continue_block);
> > > > >  > -   LLVMClearInsertionPosition(
> ctx->ac.builder);
> > > > >  > +   ac_build_continue(ctx);
> > > > >  > break;
> > > > >  > default:
> > > > >  > fprintf(stderr, "Unknown NIR jump instr: ");
> > > > >  > @@ -5285,7 +5283,7 @@ static void visit_block(struct
> > > > > ac_nir_context *ctx, nir_block *block)
> > > > >  > visit_ssa_undef(ctx,
> > > > > nir_instr_as_ssa_undef(instr));
> > > > >  > break;
> > > > >  > case nir_instr_type_jump:
> > > > >  > -   visit_jump(ctx,
> nir_instr_as_jump(instr));
> > > > >  > +   visit_jump(&ctx->ac,
> > > > > nir_instr_as_jump(instr));
> > > > >  > break;
> > > > >  > default:
> > > > >  > fprintf(stderr, "Unknown NIR instr
> type: ");
> > > > >

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

2018-04-10 Thread Juan A. Suarez Romero

On Tue, 2018-04-03 at 10:58 +0100, Alex Smith wrote:
> I don't know exactly what's causing it, no. I noticed the issue was fixed on 
> master so just bisected to this.
> 
> CC'ing stable to nominate:
> 42627dabb4db3011825a022325be7ae9b51103d6 - (1/3) ac: add if/loop build 
> helpers 
> 6e1a142863b368a032e333f09feb107241446053 - (2/3) radeonsi: make use of 
> if/loop build helpers in ac
> 99cdc019bf6fe11c135b7544ef6daf4ac964fa24 - (3/3) ac: make use of if/loop 
> build helpers
> 

Hi, Alex.

Are these 3 commits nominated for a specific stable branch? From the CC not sure
if you want to nominate them for 17.3, 18.0 or both.


J.A.

> 
> 
> On 3 April 2018 at 10:45, Timothy Arceri  wrote:
> > I have no issue with these going in stable if they fix bugs. Ideally we 
> > should create a piglit test to catch this also but presumably you guys 
> > don't actually know the exact shader combination thats tripping things up?
> > 
> > 
> > On 03/04/18 19:36, Samuel Pitoiset wrote:
> > > This fixes a rendering issue with Wolfenstein 2 as well. A backport 
> > > sounds reasonable to me.
> > > 
> > > On 04/03/2018 11:33 AM, Alex Smith wrote:
> > > > Hi Timothy,
> > > > 
> > > > This patch fixes some rendering issues I see with RADV on SI.
> > > > 
> > > > It doesn't sound like it was really intended to fix anything, so 
> > > > possibly it's masking some other issue, but would you object to 
> > > > nominating the series for stable? Applying it on the 18.0 branch fixes 
> > > > the issue there as well.
> > > > 
> > > > Thanks,
> > > > Alex
> > > > 
> > > > On 7 March 2018 at 20:43, Marek Olšák  > > > > wrote:
> > > > 
> > > > For the series:
> > > > 
> > > > Reviewed-by: Marek Olšák  > > > >
> > > > 
> > > > Marek
> > > > 
> > > > On Tue, Mar 6, 2018 at 8:40 PM, Timothy Arceri
> > > > mailto:tarc...@itsqueeze.com>> wrote:
> > > >  > These helpers insert the basic block in the same order as they
> > > >  > appear in NIR making it easier to follow LLVM IR dumps. The 
> > > > helpers
> > > >  > also insert more useful labels onto the blocks.
> > > >  >
> > > >  > TGSI use the line number of the corresponding opcode in the TGSI
> > > >  > dump as the label id, here we use the corresponding block index
> > > >  > from NIR.
> > > >  > ---
> > > >  >  src/amd/common/ac_nir_to_llvm.c | 60
> > > > +
> > > >  >  1 file changed, 18 insertions(+), 42 deletions(-)
> > > >  >
> > > >  > diff --git a/src/amd/common/ac_nir_to_llvm.c
> > > > b/src/amd/common/ac_nir_to_llvm.c
> > > >  > index cda91fe8bf..dc463ed253 100644
> > > >  > --- a/src/amd/common/ac_nir_to_llvm.c
> > > >  > +++ b/src/amd/common/ac_nir_to_llvm.c
> > > >  > @@ -5237,17 +5237,15 @@ static void visit_ssa_undef(struct
> > > > ac_nir_context *ctx,
> > > >  > _mesa_hash_table_insert(ctx->defs, &instr->def, undef);
> > > >  >  }
> > > >  >
> > > >  > -static void visit_jump(struct ac_nir_context *ctx,
> > > >  > +static void visit_jump(struct ac_llvm_context *ctx,
> > > >  >const nir_jump_instr *instr)
> > > >  >  {
> > > >  > switch (instr->type) {
> > > >  > case nir_jump_break:
> > > >  > -   LLVMBuildBr(ctx->ac.builder, ctx->break_block);
> > > >  > -   LLVMClearInsertionPosition(ctx->ac.builder);
> > > >  > +   ac_build_break(ctx);
> > > >  > break;
> > > >  > case nir_jump_continue:
> > > >  > -   LLVMBuildBr(ctx->ac.builder, 
> > > > ctx->continue_block);
> > > >  > -   LLVMClearInsertionPosition(ctx->ac.builder);
> > > >  > +   ac_build_continue(ctx);
> > > >  > break;
> > > >  > default:
> > > >  > fprintf(stderr, "Unknown NIR jump instr: ");
> > > >  > @@ -5285,7 +5283,7 @@ static void visit_block(struct
> > > > ac_nir_context *ctx, nir_block *block)
> > > >  > visit_ssa_undef(ctx,
> > > > nir_instr_as_ssa_undef(instr));
> > > >  > break;
> > > >  > case nir_instr_type_jump:
> > > >  > -   visit_jump(ctx, 
> > > > nir_instr_as_jump(instr));
> > > >  > +   visit_jump(&ctx->ac,
> > > > nir_instr_as_jump(instr));
> > > >  > break;
> > > >  > default:
> > > >  > fprintf(stderr, "Unknown NIR instr type: 
> > > > ");
> > > >  > @@ -5302,56 +5300,34 @@ static void visit_if(struct
> > > > ac_nir_context *ctx, nir_if *if_stmt)
> > > >  >  {
> > > >  > LLVMValueRef value = get_src(ctx, if_stmt->condition);
> > > >  >
> > > >  > -   LLVMValueRef fn =
> > > > LLVMGetBa

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

2018-04-03 Thread Alex Smith

I don't know exactly what's causing it, no. I noticed the issue was fixed
on master so just bisected to this.

CC'ing stable to nominate:
42627dabb4db3011825a022325be7ae9b51103d6 - (1/3) ac: add if/loop build
helpers
6e1a142863b368a032e333f09feb107241446053 - (2/3) radeonsi: make use of
if/loop build helpers in ac
99cdc019bf6fe11c135b7544ef6daf4ac964fa24 - (3/3) ac: make use of if/loop
build helpers



On 3 April 2018 at 10:45, Timothy Arceri  wrote:

> I have no issue with these going in stable if they fix bugs. Ideally we
> should create a piglit test to catch this also but presumably you guys
> don't actually know the exact shader combination thats tripping things up?
>
>
> On 03/04/18 19:36, Samuel Pitoiset wrote:
>
>> This fixes a rendering issue with Wolfenstein 2 as well. A backport
>> sounds reasonable to me.
>>
>> On 04/03/2018 11:33 AM, Alex Smith wrote:
>>
>>> Hi Timothy,
>>>
>>> This patch fixes some rendering issues I see with RADV on SI.
>>>
>>> It doesn't sound like it was really intended to fix anything, so
>>> possibly it's masking some other issue, but would you object to nominating
>>> the series for stable? Applying it on the 18.0 branch fixes the issue there
>>> as well.
>>>
>>> Thanks,
>>> Alex
>>>
>>> On 7 March 2018 at 20:43, Marek Olšák >> mar...@gmail.com>> wrote:
>>>
>>> For the series:
>>>
>>> Reviewed-by: Marek Olšák >> >
>>>
>>> Marek
>>>
>>> On Tue, Mar 6, 2018 at 8:40 PM, Timothy Arceri
>>> mailto:tarc...@itsqueeze.com>> wrote:
>>>  > These helpers insert the basic block in the same order as they
>>>  > appear in NIR making it easier to follow LLVM IR dumps. The
>>> helpers
>>>  > also insert more useful labels onto the blocks.
>>>  >
>>>  > TGSI use the line number of the corresponding opcode in the TGSI
>>>  > dump as the label id, here we use the corresponding block index
>>>  > from NIR.
>>>  > ---
>>>  >  src/amd/common/ac_nir_to_llvm.c | 60
>>> +
>>>  >  1 file changed, 18 insertions(+), 42 deletions(-)
>>>  >
>>>  > diff --git a/src/amd/common/ac_nir_to_llvm.c
>>> b/src/amd/common/ac_nir_to_llvm.c
>>>  > index cda91fe8bf..dc463ed253 100644
>>>  > --- a/src/amd/common/ac_nir_to_llvm.c
>>>  > +++ b/src/amd/common/ac_nir_to_llvm.c
>>>  > @@ -5237,17 +5237,15 @@ static void visit_ssa_undef(struct
>>> ac_nir_context *ctx,
>>>  > _mesa_hash_table_insert(ctx->defs, &instr->def, undef);
>>>  >  }
>>>  >
>>>  > -static void visit_jump(struct ac_nir_context *ctx,
>>>  > +static void visit_jump(struct ac_llvm_context *ctx,
>>>  >const nir_jump_instr *instr)
>>>  >  {
>>>  > switch (instr->type) {
>>>  > case nir_jump_break:
>>>  > -   LLVMBuildBr(ctx->ac.builder, ctx->break_block);
>>>  > -   LLVMClearInsertionPosition(ctx->ac.builder);
>>>  > +   ac_build_break(ctx);
>>>  > break;
>>>  > case nir_jump_continue:
>>>  > -   LLVMBuildBr(ctx->ac.builder, ctx->continue_block);
>>>  > -   LLVMClearInsertionPosition(ctx->ac.builder);
>>>  > +   ac_build_continue(ctx);
>>>  > break;
>>>  > default:
>>>  > fprintf(stderr, "Unknown NIR jump instr: ");
>>>  > @@ -5285,7 +5283,7 @@ static void visit_block(struct
>>> ac_nir_context *ctx, nir_block *block)
>>>  > visit_ssa_undef(ctx,
>>> nir_instr_as_ssa_undef(instr));
>>>  > break;
>>>  > case nir_instr_type_jump:
>>>  > -   visit_jump(ctx, nir_instr_as_jump(instr));
>>>  > +   visit_jump(&ctx->ac,
>>> nir_instr_as_jump(instr));
>>>  > break;
>>>  > default:
>>>  > fprintf(stderr, "Unknown NIR instr type:
>>> ");
>>>  > @@ -5302,56 +5300,34 @@ static void visit_if(struct
>>> ac_nir_context *ctx, nir_if *if_stmt)
>>>  >  {
>>>  > LLVMValueRef value = get_src(ctx, if_stmt->condition);
>>>  >
>>>  > -   LLVMValueRef fn =
>>> LLVMGetBasicBlockParent(LLVMGetInsertBlock(ctx->ac.builder));
>>>  > -   LLVMBasicBlockRef merge_block =
>>>  > -   LLVMAppendBasicBlockInContext(ctx->ac.context, fn,
>>> "");
>>>  > -   LLVMBasicBlockRef if_block =
>>>  > -   LLVMAppendBasicBlockInContext(ctx->ac.context, fn,
>>> "");
>>>  > -   LLVMBasicBlockRef else_block = merge_block;
>>>  > -   if (!exec_list_is_empty(&if_stmt->else_list))
>>>  > -   else_block = LLVMAppendBasicBlockInContext(
>>>  > -   ctx->ac.context, fn, "");
>>>  > -
>>>  > -   LLVMValueRef cond = LLVMBuildICmp(ctx->ac

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

2018-04-03 Thread Timothy Arceri

I have no issue with these going in stable if they fix bugs. Ideally we 
should create a piglit test to catch this also but presumably you guys 
don't actually know the exact shader combination thats tripping things up?


On 03/04/18 19:36, Samuel Pitoiset wrote:
This fixes a rendering issue with Wolfenstein 2 as well. A backport 
sounds reasonable to me.


On 04/03/2018 11:33 AM, Alex Smith wrote:

Hi Timothy,

This patch fixes some rendering issues I see with RADV on SI.

It doesn't sound like it was really intended to fix anything, so 
possibly it's masking some other issue, but would you object to 
nominating the series for stable? Applying it on the 18.0 branch fixes 
the issue there as well.


Thanks,
Alex

On 7 March 2018 at 20:43, Marek Olšák > wrote:


    For the series:

    Reviewed-by: Marek Olšák mailto:marek.ol...@amd.com>>

    Marek

    On Tue, Mar 6, 2018 at 8:40 PM, Timothy Arceri
    mailto:tarc...@itsqueeze.com>> wrote:
 > These helpers insert the basic block in the same order as they
 > appear in NIR making it easier to follow LLVM IR dumps. The 
helpers

 > also insert more useful labels onto the blocks.
 >
 > TGSI use the line number of the corresponding opcode in the TGSI
 > dump as the label id, here we use the corresponding block index
 > from NIR.
 > ---
 >  src/amd/common/ac_nir_to_llvm.c | 60
    +
 >  1 file changed, 18 insertions(+), 42 deletions(-)
 >
 > diff --git a/src/amd/common/ac_nir_to_llvm.c
    b/src/amd/common/ac_nir_to_llvm.c
 > index cda91fe8bf..dc463ed253 100644
 > --- a/src/amd/common/ac_nir_to_llvm.c
 > +++ b/src/amd/common/ac_nir_to_llvm.c
 > @@ -5237,17 +5237,15 @@ static void visit_ssa_undef(struct
    ac_nir_context *ctx,
 >         _mesa_hash_table_insert(ctx->defs, &instr->def, undef);
 >  }
 >
 > -static void visit_jump(struct ac_nir_context *ctx,
 > +static void visit_jump(struct ac_llvm_context *ctx,
 >                        const nir_jump_instr *instr)
 >  {
 >         switch (instr->type) {
 >         case nir_jump_break:
 > -               LLVMBuildBr(ctx->ac.builder, ctx->break_block);
 > -               LLVMClearInsertionPosition(ctx->ac.builder);
 > +               ac_build_break(ctx);
 >                 break;
 >         case nir_jump_continue:
 > -               LLVMBuildBr(ctx->ac.builder, ctx->continue_block);
 > -               LLVMClearInsertionPosition(ctx->ac.builder);
 > +               ac_build_continue(ctx);
 >                 break;
 >         default:
 >                 fprintf(stderr, "Unknown NIR jump instr: ");
 > @@ -5285,7 +5283,7 @@ static void visit_block(struct
    ac_nir_context *ctx, nir_block *block)
 >                         visit_ssa_undef(ctx,
    nir_instr_as_ssa_undef(instr));
 >                         break;
 >                 case nir_instr_type_jump:
 > -                       visit_jump(ctx, nir_instr_as_jump(instr));
 > +                       visit_jump(&ctx->ac,
    nir_instr_as_jump(instr));
 >                         break;
 >                 default:
 >                         fprintf(stderr, "Unknown NIR instr 
type: ");

 > @@ -5302,56 +5300,34 @@ static void visit_if(struct
    ac_nir_context *ctx, nir_if *if_stmt)
 >  {
 >         LLVMValueRef value = get_src(ctx, if_stmt->condition);
 >
 > -       LLVMValueRef fn =
    LLVMGetBasicBlockParent(LLVMGetInsertBlock(ctx->ac.builder));
 > -       LLVMBasicBlockRef merge_block =
 > -           LLVMAppendBasicBlockInContext(ctx->ac.context, fn, 
"");

 > -       LLVMBasicBlockRef if_block =
 > -           LLVMAppendBasicBlockInContext(ctx->ac.context, fn, 
"");

 > -       LLVMBasicBlockRef else_block = merge_block;
 > -       if (!exec_list_is_empty(&if_stmt->else_list))
 > -               else_block = LLVMAppendBasicBlockInContext(
 > -                   ctx->ac.context, fn, "");
 > -
 > -       LLVMValueRef cond = LLVMBuildICmp(ctx->ac.builder,
    LLVMIntNE, value,
 > -                                         ctx->ac.i32_0, "");
 > -       LLVMBuildCondBr(ctx->ac.builder, cond, if_block, 
else_block);

 > -
 > -       LLVMPositionBuilderAtEnd(ctx->ac.builder, if_block);
 > +       nir_block *then_block =
 > +               (nir_block *)
    exec_list_get_head(&if_stmt->then_list);
 > +
 > +       ac_build_uif(&ctx->ac, value, then_block->index);
 > +
 >         visit_cf_list(ctx, &if_stmt->then_list);
 > -       if (LLVMGetInsertBlock(ctx->ac.builder))
 > -               LLVMBuildBr(ctx->ac.builder, merge_block);
 >
 >         if (!exec_list_is_empty(&if_stmt->else_list)) {
 > -               LLVMPositionBuilderAtEnd(ctx->ac.builder,
    else_block);
 > +               nir_block *else_block

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

2018-04-03 Thread Samuel Pitoiset

This fixes a rendering issue with Wolfenstein 2 as well. A backport 
sounds reasonable to me.


On 04/03/2018 11:33 AM, Alex Smith wrote:

Hi Timothy,

This patch fixes some rendering issues I see with RADV on SI.

It doesn't sound like it was really intended to fix anything, so 
possibly it's masking some other issue, but would you object to 
nominating the series for stable? Applying it on the 18.0 branch fixes 
the issue there as well.


Thanks,
Alex

On 7 March 2018 at 20:43, Marek Olšák > wrote:


For the series:

Reviewed-by: Marek Olšák mailto:marek.ol...@amd.com>>

Marek

On Tue, Mar 6, 2018 at 8:40 PM, Timothy Arceri
mailto:tarc...@itsqueeze.com>> wrote:
 > These helpers insert the basic block in the same order as they
 > appear in NIR making it easier to follow LLVM IR dumps. The helpers
 > also insert more useful labels onto the blocks.
 >
 > TGSI use the line number of the corresponding opcode in the TGSI
 > dump as the label id, here we use the corresponding block index
 > from NIR.
 > ---
 >  src/amd/common/ac_nir_to_llvm.c | 60
+
 >  1 file changed, 18 insertions(+), 42 deletions(-)
 >
 > diff --git a/src/amd/common/ac_nir_to_llvm.c
b/src/amd/common/ac_nir_to_llvm.c
 > index cda91fe8bf..dc463ed253 100644
 > --- a/src/amd/common/ac_nir_to_llvm.c
 > +++ b/src/amd/common/ac_nir_to_llvm.c
 > @@ -5237,17 +5237,15 @@ static void visit_ssa_undef(struct
ac_nir_context *ctx,
 >         _mesa_hash_table_insert(ctx->defs, &instr->def, undef);
 >  }
 >
 > -static void visit_jump(struct ac_nir_context *ctx,
 > +static void visit_jump(struct ac_llvm_context *ctx,
 >                        const nir_jump_instr *instr)
 >  {
 >         switch (instr->type) {
 >         case nir_jump_break:
 > -               LLVMBuildBr(ctx->ac.builder, ctx->break_block);
 > -               LLVMClearInsertionPosition(ctx->ac.builder);
 > +               ac_build_break(ctx);
 >                 break;
 >         case nir_jump_continue:
 > -               LLVMBuildBr(ctx->ac.builder, ctx->continue_block);
 > -               LLVMClearInsertionPosition(ctx->ac.builder);
 > +               ac_build_continue(ctx);
 >                 break;
 >         default:
 >                 fprintf(stderr, "Unknown NIR jump instr: ");
 > @@ -5285,7 +5283,7 @@ static void visit_block(struct
ac_nir_context *ctx, nir_block *block)
 >                         visit_ssa_undef(ctx,
nir_instr_as_ssa_undef(instr));
 >                         break;
 >                 case nir_instr_type_jump:
 > -                       visit_jump(ctx, nir_instr_as_jump(instr));
 > +                       visit_jump(&ctx->ac,
nir_instr_as_jump(instr));
 >                         break;
 >                 default:
 >                         fprintf(stderr, "Unknown NIR instr type: ");
 > @@ -5302,56 +5300,34 @@ static void visit_if(struct
ac_nir_context *ctx, nir_if *if_stmt)
 >  {
 >         LLVMValueRef value = get_src(ctx, if_stmt->condition);
 >
 > -       LLVMValueRef fn =
LLVMGetBasicBlockParent(LLVMGetInsertBlock(ctx->ac.builder));
 > -       LLVMBasicBlockRef merge_block =
 > -           LLVMAppendBasicBlockInContext(ctx->ac.context, fn, "");
 > -       LLVMBasicBlockRef if_block =
 > -           LLVMAppendBasicBlockInContext(ctx->ac.context, fn, "");
 > -       LLVMBasicBlockRef else_block = merge_block;
 > -       if (!exec_list_is_empty(&if_stmt->else_list))
 > -               else_block = LLVMAppendBasicBlockInContext(
 > -                   ctx->ac.context, fn, "");
 > -
 > -       LLVMValueRef cond = LLVMBuildICmp(ctx->ac.builder,
LLVMIntNE, value,
 > -                                         ctx->ac.i32_0, "");
 > -       LLVMBuildCondBr(ctx->ac.builder, cond, if_block, else_block);
 > -
 > -       LLVMPositionBuilderAtEnd(ctx->ac.builder, if_block);
 > +       nir_block *then_block =
 > +               (nir_block *)
exec_list_get_head(&if_stmt->then_list);
 > +
 > +       ac_build_uif(&ctx->ac, value, then_block->index);
 > +
 >         visit_cf_list(ctx, &if_stmt->then_list);
 > -       if (LLVMGetInsertBlock(ctx->ac.builder))
 > -               LLVMBuildBr(ctx->ac.builder, merge_block);
 >
 >         if (!exec_list_is_empty(&if_stmt->else_list)) {
 > -               LLVMPositionBuilderAtEnd(ctx->ac.builder,
else_block);
 > +               nir_block *else_block =
 > +                       (nir_block *)
exec_list_get_head(&if_stmt->else_list);
 > +
 > +               ac_build_else(&ctx->ac, else_block->index);
 >                 visit_cf_list(ctx, &if_stmt->else_list);
 > -               if (LLVMGetInsertB

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

2018-04-03 Thread Alex Smith

Hi Timothy,

This patch fixes some rendering issues I see with RADV on SI.

It doesn't sound like it was really intended to fix anything, so possibly
it's masking some other issue, but would you object to nominating the
series for stable? Applying it on the 18.0 branch fixes the issue there as
well.

Thanks,
Alex

On 7 March 2018 at 20:43, Marek Olšák  wrote:

> For the series:
>
> Reviewed-by: Marek Olšák 
>
> Marek
>
> On Tue, Mar 6, 2018 at 8:40 PM, Timothy Arceri 
> wrote:
> > These helpers insert the basic block in the same order as they
> > appear in NIR making it easier to follow LLVM IR dumps. The helpers
> > also insert more useful labels onto the blocks.
> >
> > TGSI use the line number of the corresponding opcode in the TGSI
> > dump as the label id, here we use the corresponding block index
> > from NIR.
> > ---
> >  src/amd/common/ac_nir_to_llvm.c | 60 +-
> ---
> >  1 file changed, 18 insertions(+), 42 deletions(-)
> >
> > diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_
> llvm.c
> > index cda91fe8bf..dc463ed253 100644
> > --- a/src/amd/common/ac_nir_to_llvm.c
> > +++ b/src/amd/common/ac_nir_to_llvm.c
> > @@ -5237,17 +5237,15 @@ static void visit_ssa_undef(struct
> ac_nir_context *ctx,
> > _mesa_hash_table_insert(ctx->defs, &instr->def, undef);
> >  }
> >
> > -static void visit_jump(struct ac_nir_context *ctx,
> > +static void visit_jump(struct ac_llvm_context *ctx,
> >const nir_jump_instr *instr)
> >  {
> > switch (instr->type) {
> > case nir_jump_break:
> > -   LLVMBuildBr(ctx->ac.builder, ctx->break_block);
> > -   LLVMClearInsertionPosition(ctx->ac.builder);
> > +   ac_build_break(ctx);
> > break;
> > case nir_jump_continue:
> > -   LLVMBuildBr(ctx->ac.builder, ctx->continue_block);
> > -   LLVMClearInsertionPosition(ctx->ac.builder);
> > +   ac_build_continue(ctx);
> > break;
> > default:
> > fprintf(stderr, "Unknown NIR jump instr: ");
> > @@ -5285,7 +5283,7 @@ static void visit_block(struct ac_nir_context
> *ctx, nir_block *block)
> > visit_ssa_undef(ctx,
> nir_instr_as_ssa_undef(instr));
> > break;
> > case nir_instr_type_jump:
> > -   visit_jump(ctx, nir_instr_as_jump(instr));
> > +   visit_jump(&ctx->ac, nir_instr_as_jump(instr));
> > break;
> > default:
> > fprintf(stderr, "Unknown NIR instr type: ");
> > @@ -5302,56 +5300,34 @@ static void visit_if(struct ac_nir_context *ctx,
> nir_if *if_stmt)
> >  {
> > LLVMValueRef value = get_src(ctx, if_stmt->condition);
> >
> > -   LLVMValueRef fn = LLVMGetBasicBlockParent(
> LLVMGetInsertBlock(ctx->ac.builder));
> > -   LLVMBasicBlockRef merge_block =
> > -   LLVMAppendBasicBlockInContext(ctx->ac.context, fn, "");
> > -   LLVMBasicBlockRef if_block =
> > -   LLVMAppendBasicBlockInContext(ctx->ac.context, fn, "");
> > -   LLVMBasicBlockRef else_block = merge_block;
> > -   if (!exec_list_is_empty(&if_stmt->else_list))
> > -   else_block = LLVMAppendBasicBlockInContext(
> > -   ctx->ac.context, fn, "");
> > -
> > -   LLVMValueRef cond = LLVMBuildICmp(ctx->ac.builder, LLVMIntNE,
> value,
> > - ctx->ac.i32_0, "");
> > -   LLVMBuildCondBr(ctx->ac.builder, cond, if_block, else_block);
> > -
> > -   LLVMPositionBuilderAtEnd(ctx->ac.builder, if_block);
> > +   nir_block *then_block =
> > +   (nir_block *) exec_list_get_head(&if_stmt->then_list);
> > +
> > +   ac_build_uif(&ctx->ac, value, then_block->index);
> > +
> > visit_cf_list(ctx, &if_stmt->then_list);
> > -   if (LLVMGetInsertBlock(ctx->ac.builder))
> > -   LLVMBuildBr(ctx->ac.builder, merge_block);
> >
> > if (!exec_list_is_empty(&if_stmt->else_list)) {
> > -   LLVMPositionBuilderAtEnd(ctx->ac.builder, else_block);
> > +   nir_block *else_block =
> > +   (nir_block *) exec_list_get_head(&if_stmt->
> else_list);
> > +
> > +   ac_build_else(&ctx->ac, else_block->index);
> > visit_cf_list(ctx, &if_stmt->else_list);
> > -   if (LLVMGetInsertBlock(ctx->ac.builder))
> > -   LLVMBuildBr(ctx->ac.builder, merge_block);
> > }
> >
> > -   LLVMPositionBuilderAtEnd(ctx->ac.builder, merge_block);
> > +   ac_build_endif(&ctx->ac, then_block->index);
> >  }
> >
> >  static void visit_loop(struct ac_nir_context *ctx, nir_loop *loop)
> >  {
> > -   LLVMValueRef fn = LLVMGetBasicBlockParent(
> LLVMGetInsertBlock(ctx->ac.builder));
> > -   LLVMBasicBlockRef continue_parent = ctx->continue_block;
> > -   L

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

2018-03-07 Thread Marek Olšák

For the series:

Reviewed-by: Marek Olšák 

Marek

On Tue, Mar 6, 2018 at 8:40 PM, Timothy Arceri  wrote:
> These helpers insert the basic block in the same order as they
> appear in NIR making it easier to follow LLVM IR dumps. The helpers
> also insert more useful labels onto the blocks.
>
> TGSI use the line number of the corresponding opcode in the TGSI
> dump as the label id, here we use the corresponding block index
> from NIR.
> ---
>  src/amd/common/ac_nir_to_llvm.c | 60 
> +
>  1 file changed, 18 insertions(+), 42 deletions(-)
>
> diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
> index cda91fe8bf..dc463ed253 100644
> --- a/src/amd/common/ac_nir_to_llvm.c
> +++ b/src/amd/common/ac_nir_to_llvm.c
> @@ -5237,17 +5237,15 @@ static void visit_ssa_undef(struct ac_nir_context 
> *ctx,
> _mesa_hash_table_insert(ctx->defs, &instr->def, undef);
>  }
>
> -static void visit_jump(struct ac_nir_context *ctx,
> +static void visit_jump(struct ac_llvm_context *ctx,
>const nir_jump_instr *instr)
>  {
> switch (instr->type) {
> case nir_jump_break:
> -   LLVMBuildBr(ctx->ac.builder, ctx->break_block);
> -   LLVMClearInsertionPosition(ctx->ac.builder);
> +   ac_build_break(ctx);
> break;
> case nir_jump_continue:
> -   LLVMBuildBr(ctx->ac.builder, ctx->continue_block);
> -   LLVMClearInsertionPosition(ctx->ac.builder);
> +   ac_build_continue(ctx);
> break;
> default:
> fprintf(stderr, "Unknown NIR jump instr: ");
> @@ -5285,7 +5283,7 @@ static void visit_block(struct ac_nir_context *ctx, 
> nir_block *block)
> visit_ssa_undef(ctx, nir_instr_as_ssa_undef(instr));
> break;
> case nir_instr_type_jump:
> -   visit_jump(ctx, nir_instr_as_jump(instr));
> +   visit_jump(&ctx->ac, nir_instr_as_jump(instr));
> break;
> default:
> fprintf(stderr, "Unknown NIR instr type: ");
> @@ -5302,56 +5300,34 @@ static void visit_if(struct ac_nir_context *ctx, 
> nir_if *if_stmt)
>  {
> LLVMValueRef value = get_src(ctx, if_stmt->condition);
>
> -   LLVMValueRef fn = 
> LLVMGetBasicBlockParent(LLVMGetInsertBlock(ctx->ac.builder));
> -   LLVMBasicBlockRef merge_block =
> -   LLVMAppendBasicBlockInContext(ctx->ac.context, fn, "");
> -   LLVMBasicBlockRef if_block =
> -   LLVMAppendBasicBlockInContext(ctx->ac.context, fn, "");
> -   LLVMBasicBlockRef else_block = merge_block;
> -   if (!exec_list_is_empty(&if_stmt->else_list))
> -   else_block = LLVMAppendBasicBlockInContext(
> -   ctx->ac.context, fn, "");
> -
> -   LLVMValueRef cond = LLVMBuildICmp(ctx->ac.builder, LLVMIntNE, value,
> - ctx->ac.i32_0, "");
> -   LLVMBuildCondBr(ctx->ac.builder, cond, if_block, else_block);
> -
> -   LLVMPositionBuilderAtEnd(ctx->ac.builder, if_block);
> +   nir_block *then_block =
> +   (nir_block *) exec_list_get_head(&if_stmt->then_list);
> +
> +   ac_build_uif(&ctx->ac, value, then_block->index);
> +
> visit_cf_list(ctx, &if_stmt->then_list);
> -   if (LLVMGetInsertBlock(ctx->ac.builder))
> -   LLVMBuildBr(ctx->ac.builder, merge_block);
>
> if (!exec_list_is_empty(&if_stmt->else_list)) {
> -   LLVMPositionBuilderAtEnd(ctx->ac.builder, else_block);
> +   nir_block *else_block =
> +   (nir_block *) exec_list_get_head(&if_stmt->else_list);
> +
> +   ac_build_else(&ctx->ac, else_block->index);
> visit_cf_list(ctx, &if_stmt->else_list);
> -   if (LLVMGetInsertBlock(ctx->ac.builder))
> -   LLVMBuildBr(ctx->ac.builder, merge_block);
> }
>
> -   LLVMPositionBuilderAtEnd(ctx->ac.builder, merge_block);
> +   ac_build_endif(&ctx->ac, then_block->index);
>  }
>
>  static void visit_loop(struct ac_nir_context *ctx, nir_loop *loop)
>  {
> -   LLVMValueRef fn = 
> LLVMGetBasicBlockParent(LLVMGetInsertBlock(ctx->ac.builder));
> -   LLVMBasicBlockRef continue_parent = ctx->continue_block;
> -   LLVMBasicBlockRef break_parent = ctx->break_block;
> +   nir_block *first_loop_block =
> +   (nir_block *) exec_list_get_head(&loop->body);
>
> -   ctx->continue_block =
> -   LLVMAppendBasicBlockInContext(ctx->ac.context, fn, "");
> -   ctx->break_block =
> -   LLVMAppendBasicBlockInContext(ctx->ac.context, fn, "");
> +   ac_build_bgnloop(&ctx->ac, first_loop_block->index);
>
> -   LLVMBuildBr(ctx->ac.builder, ctx->continue_block);
> -   LLVMPositionBuilderAtEnd(ctx->ac.builder, ctx->continue_block);
> v

[Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

2018-03-06 Thread Timothy Arceri

These helpers insert the basic block in the same order as they
appear in NIR making it easier to follow LLVM IR dumps. The helpers
also insert more useful labels onto the blocks.

TGSI use the line number of the corresponding opcode in the TGSI
dump as the label id, here we use the corresponding block index
from NIR.
---
 src/amd/common/ac_nir_to_llvm.c | 60 +
 1 file changed, 18 insertions(+), 42 deletions(-)

diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index cda91fe8bf..dc463ed253 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -5237,17 +5237,15 @@ static void visit_ssa_undef(struct ac_nir_context *ctx,
_mesa_hash_table_insert(ctx->defs, &instr->def, undef);
 }
 
-static void visit_jump(struct ac_nir_context *ctx,
+static void visit_jump(struct ac_llvm_context *ctx,
   const nir_jump_instr *instr)
 {
switch (instr->type) {
case nir_jump_break:
-   LLVMBuildBr(ctx->ac.builder, ctx->break_block);
-   LLVMClearInsertionPosition(ctx->ac.builder);
+   ac_build_break(ctx);
break;
case nir_jump_continue:
-   LLVMBuildBr(ctx->ac.builder, ctx->continue_block);
-   LLVMClearInsertionPosition(ctx->ac.builder);
+   ac_build_continue(ctx);
break;
default:
fprintf(stderr, "Unknown NIR jump instr: ");
@@ -5285,7 +5283,7 @@ static void visit_block(struct ac_nir_context *ctx, 
nir_block *block)
visit_ssa_undef(ctx, nir_instr_as_ssa_undef(instr));
break;
case nir_instr_type_jump:
-   visit_jump(ctx, nir_instr_as_jump(instr));
+   visit_jump(&ctx->ac, nir_instr_as_jump(instr));
break;
default:
fprintf(stderr, "Unknown NIR instr type: ");
@@ -5302,56 +5300,34 @@ static void visit_if(struct ac_nir_context *ctx, nir_if 
*if_stmt)
 {
LLVMValueRef value = get_src(ctx, if_stmt->condition);
 
-   LLVMValueRef fn = 
LLVMGetBasicBlockParent(LLVMGetInsertBlock(ctx->ac.builder));
-   LLVMBasicBlockRef merge_block =
-   LLVMAppendBasicBlockInContext(ctx->ac.context, fn, "");
-   LLVMBasicBlockRef if_block =
-   LLVMAppendBasicBlockInContext(ctx->ac.context, fn, "");
-   LLVMBasicBlockRef else_block = merge_block;
-   if (!exec_list_is_empty(&if_stmt->else_list))
-   else_block = LLVMAppendBasicBlockInContext(
-   ctx->ac.context, fn, "");
-
-   LLVMValueRef cond = LLVMBuildICmp(ctx->ac.builder, LLVMIntNE, value,
- ctx->ac.i32_0, "");
-   LLVMBuildCondBr(ctx->ac.builder, cond, if_block, else_block);
-
-   LLVMPositionBuilderAtEnd(ctx->ac.builder, if_block);
+   nir_block *then_block =
+   (nir_block *) exec_list_get_head(&if_stmt->then_list);
+
+   ac_build_uif(&ctx->ac, value, then_block->index);
+
visit_cf_list(ctx, &if_stmt->then_list);
-   if (LLVMGetInsertBlock(ctx->ac.builder))
-   LLVMBuildBr(ctx->ac.builder, merge_block);
 
if (!exec_list_is_empty(&if_stmt->else_list)) {
-   LLVMPositionBuilderAtEnd(ctx->ac.builder, else_block);
+   nir_block *else_block =
+   (nir_block *) exec_list_get_head(&if_stmt->else_list);
+
+   ac_build_else(&ctx->ac, else_block->index);
visit_cf_list(ctx, &if_stmt->else_list);
-   if (LLVMGetInsertBlock(ctx->ac.builder))
-   LLVMBuildBr(ctx->ac.builder, merge_block);
}
 
-   LLVMPositionBuilderAtEnd(ctx->ac.builder, merge_block);
+   ac_build_endif(&ctx->ac, then_block->index);
 }
 
 static void visit_loop(struct ac_nir_context *ctx, nir_loop *loop)
 {
-   LLVMValueRef fn = 
LLVMGetBasicBlockParent(LLVMGetInsertBlock(ctx->ac.builder));
-   LLVMBasicBlockRef continue_parent = ctx->continue_block;
-   LLVMBasicBlockRef break_parent = ctx->break_block;
+   nir_block *first_loop_block =
+   (nir_block *) exec_list_get_head(&loop->body);
 
-   ctx->continue_block =
-   LLVMAppendBasicBlockInContext(ctx->ac.context, fn, "");
-   ctx->break_block =
-   LLVMAppendBasicBlockInContext(ctx->ac.context, fn, "");
+   ac_build_bgnloop(&ctx->ac, first_loop_block->index);
 
-   LLVMBuildBr(ctx->ac.builder, ctx->continue_block);
-   LLVMPositionBuilderAtEnd(ctx->ac.builder, ctx->continue_block);
visit_cf_list(ctx, &loop->body);
 
-   if (LLVMGetInsertBlock(ctx->ac.builder))
-   LLVMBuildBr(ctx->ac.builder, ctx->continue_block);
-   LLVMPositionBuilderAtEnd(ctx->ac.builder, ctx->break_block);
-
-   ctx->continue_block = continue_parent;
-   ctx->break_block = break_parent;
+

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

Re: [Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

[Mesa-dev] [PATCH 3/3] ac: make use of if/loop build helpers

8 matches

Site Navigation

Mail list logo

Footer information