[1/2] panfrost: Don't use mir_foreach_instr_in_block_safe() when not needed

Submitted by Boris Brezillon on Aug. 13, 2019, 7:25 p.m.

Details

Message ID 20190813192545.13349-1-boris.brezillon@collabora.com
State New
Headers show
Series "Series without cover letter" ( rev: 2 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Boris Brezillon Aug. 13, 2019, 7:25 p.m.
mir_foreach_instr_in_block_safe() is only needed if the caller intend
to remove the current item from the list. Downgrade to
mir_foreach_instr_in_block() when this is not the case.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 src/panfrost/midgard/midgard_compile.c         | 2 +-
 src/panfrost/midgard/midgard_opt_invert.c      | 6 +++---
 src/panfrost/midgard/midgard_opt_perspective.c | 6 +++---
 src/panfrost/midgard/midgard_schedule.c        | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/panfrost/midgard/midgard_compile.c b/src/panfrost/midgard/midgard_compile.c
index 460847e82660..e261e80c06c5 100644
--- a/src/panfrost/midgard/midgard_compile.c
+++ b/src/panfrost/midgard/midgard_compile.c
@@ -2161,7 +2161,7 @@  midgard_opt_pos_propagate(compiler_context *ctx, midgard_block *block)
 {
         bool progress = false;
 
-        mir_foreach_instr_in_block_safe(block, ins) {
+        mir_foreach_instr_in_block(block, ins) {
                 if (ins->type != TAG_ALU_4) continue;
                 if (ins->alu.op != midgard_alu_op_fmov) continue;
                 if (ins->alu.outmod != midgard_outmod_pos) continue;
diff --git a/src/panfrost/midgard/midgard_opt_invert.c b/src/panfrost/midgard/midgard_opt_invert.c
index b68c98c74dbb..422be6ef03e5 100644
--- a/src/panfrost/midgard/midgard_opt_invert.c
+++ b/src/panfrost/midgard/midgard_opt_invert.c
@@ -69,7 +69,7 @@  midgard_opt_not_propagate(compiler_context *ctx, midgard_block *block)
 {
         bool progress = false;
 
-        mir_foreach_instr_in_block_safe(block, ins) {
+        mir_foreach_instr_in_block(block, ins) {
                 if (ins->type != TAG_ALU_4) continue;
                 if (ins->alu.op != midgard_alu_op_imov) continue;
                 if (!ins->invert) continue;
@@ -162,7 +162,7 @@  midgard_opt_fuse_dest_invert(compiler_context *ctx, midgard_block *block)
 {
         bool progress = false;
 
-        mir_foreach_instr_in_block_safe(block, ins) {
+        mir_foreach_instr_in_block(block, ins) {
                 /* Search for inverted bitwise */
                 if (ins->type != TAG_ALU_4) continue;
                 if (!mir_is_bitwise(ins)) continue;
@@ -213,7 +213,7 @@  midgard_opt_fuse_src_invert(compiler_context *ctx, midgard_block *block)
 {
         bool progress = false;
 
-        mir_foreach_instr_in_block_safe(block, ins) {
+        mir_foreach_instr_in_block(block, ins) {
                 /* Search for inverted bitwise */
                 if (ins->type != TAG_ALU_4) continue;
                 if (!mir_is_bitwise(ins)) continue;
diff --git a/src/panfrost/midgard/midgard_opt_perspective.c b/src/panfrost/midgard/midgard_opt_perspective.c
index f5e483e68d0e..993000923b93 100644
--- a/src/panfrost/midgard/midgard_opt_perspective.c
+++ b/src/panfrost/midgard/midgard_opt_perspective.c
@@ -71,7 +71,7 @@  midgard_opt_combine_projection(compiler_context *ctx, midgard_block *block)
                 unsigned frcp_component = 0;
                 unsigned frcp_from = 0;
 
-                mir_foreach_instr_in_block_safe(block, sub) {
+                mir_foreach_instr_in_block(block, sub) {
                         if (sub->ssa_args.dest != frcp) continue;
 
                         midgard_vector_alu_src s =
@@ -97,7 +97,7 @@  midgard_opt_combine_projection(compiler_context *ctx, midgard_block *block)
                 /* One for frcp and one for fmul */
                 if (mir_use_count(ctx, frcp_from) > 2) continue;
 
-                mir_foreach_instr_in_block_safe(block, v) {
+                mir_foreach_instr_in_block(block, v) {
                         if (v->ssa_args.dest != frcp_from) continue;
                         if (v->type != TAG_LOAD_STORE_4) break;
                         if (!OP_IS_LOAD_VARY_F(v->load_store.op)) break;
@@ -157,7 +157,7 @@  midgard_opt_varying_projection(compiler_context *ctx, midgard_block *block)
 
                 bool rewritten = false;
 
-                mir_foreach_instr_in_block_safe(block, v) {
+                mir_foreach_instr_in_block(block, v) {
                         if (v->ssa_args.dest != vary) continue;
                         if (v->type != TAG_LOAD_STORE_4) break;
                         if (!OP_IS_LOAD_VARY_F(v->load_store.op)) break;
diff --git a/src/panfrost/midgard/midgard_schedule.c b/src/panfrost/midgard/midgard_schedule.c
index 486bb38e0493..7ec0d4428a43 100644
--- a/src/panfrost/midgard/midgard_schedule.c
+++ b/src/panfrost/midgard/midgard_schedule.c
@@ -806,7 +806,7 @@  static void mir_spill_register(
         /* For special reads, figure out how many components we need */
         unsigned read_mask = 0;
 
-        mir_foreach_instr_global_safe(ctx, ins) {
+        mir_foreach_instr_global(ctx, ins) {
                 read_mask |= mir_mask_of_read_components(ins, spill_node);
         }
 

Comments


On Tue, 13 Aug 2019 12:55:57 -0700
Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> wrote:

> > +++ b/src/panfrost/midgard/midgard_opt_perspective.c
> > @@ -71,7 +71,7 @@ midgard_opt_combine_projection(compiler_context *ctx, midgard_block *block)
> >                  unsigned frcp_component = 0;
> >                  unsigned frcp_from = 0;
> >  
> > -                mir_foreach_instr_in_block_safe(block, sub) {
> > +                mir_foreach_instr_in_block(block, sub) {  
> 
> Should this one be _safe?

I don't see any remove_ins()/insert_before() call in there. Do you see
any reason for using the _safe() variant here?

On Tue, 13 Aug 2019 15:30:20 -0700
Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> wrote:

> > I don't see any remove_ins()/insert_before() call in there. Do you see
> > any reason for using the _safe() variant here?  
> 
> Er, I think I meant the outer loop, which has a remove/insert pair at
> the bottom to change the current instruction.

Should I apply this patch or do you prefer to keep using the _safe()
variant even if it's not needed?

On Tue, 27 Aug 2019 07:25:03 -0700
Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> wrote:

> Assuming it passes CI, feel free to apply it :)

I guess that stands for a R-b :-).

> 
> On Tue, Aug 27, 2019 at 12:40:08PM +0200, Boris Brezillon wrote:
> > On Tue, 13 Aug 2019 15:30:20 -0700
> > Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> wrote:
> >   
> > > > I don't see any remove_ins()/insert_before() call in there. Do you see
> > > > any reason for using the _safe() variant here?    
> > > 
> > > Er, I think I meant the outer loop, which has a remove/insert pair at
> > > the bottom to change the current instruction.  
> > 
> > Should I apply this patch or do you prefer to keep using the _safe()
> > variant even if it's not needed?