Add RemoveControlFlowPass

[elhewaty]

No description provided.

[elhewaty]

Hi @asl,
I am trying to work on this pass it has a lot of bugs I think. I am trying the following case

module {
  // No need to check stuff. If it parses, it's fine.
  // CHECK: module
  %0 = p4hir.const #p4hir.bool<false> : !p4hir.bool
  p4hir.if %0 {
    %29 = p4hir.const #p4hir.bool<true> : !p4hir.bool
  } else {
    %29 = p4hir.const #p4hir.bool<true> : !p4hir.bool
  }
}

It doesn't change it at this point, but when changing a lot of things it complains about boolean condition for castop, also empty block of switch. please leave some comments to make this work.

[asl]

The parser control flow simplification pass, as stated in the name, is expected to work on ... parsers (!). There is very little (if not negative) sense to convert ordinary conditions to switches either.

Have you tried to read the description at https://github.com/p4lang/p4c/blob/main/frontends/p4/parserControlFlow.h#L27 ?

[xl4624]

Another thing to watch out for is that in P4C this pass requires all declarations to be at the "top", basically moving them all before the first state, with moveDeclaration pass (which itself requires uniqueNames to avoid multiple declarations of the same variable name used in different scopes, I think?)

I looked into this before and ran into issues with something like (dumb example):

parser p() {
	state start {
		bool b = true;
		if (b) {
			b = false;
		}
		statement;
		use b;
  }
}

But if you ran this example without any processing you'd get:

parser p() {
	state start {
		bool b = true;
		transition select (b) {
			true : start_true;
			_ : start_join;
		}
	}
	state start_true {
		b = false; // b is not defined here (same in mlir)
	}
	state start_join {
		statement;
		use b; // again can't use b
	}
}

Good luck!

[asl]

Another thing to watch out for is that in P4C this pass requires all declarations to be at the "top", basically moving them all before the first state, with moveDeclaration pass

This is just the limitation of P4C pass (as there is no sane way to track normal def-use chains there). We do not need all declarations to be moved there, only those variables that are defined in the "top" part of the state and used in the "bottom" one.

[elhewaty]

This is just the limitation of P4C pass (as there is no sane way to track normal def-use chains there). We do not need all declarations to be moved there, only those variables that are defined in the "top" part of the state and used in the "bottom" one.

@asl I am thinking of implementing two passes for renaming and hoisting vars, what do you think?

[asl]

@asl I am thinking of implementing two passes for renaming and hoisting vars, what do you think?

Not sure why something like this is needed. Maybe you'd better start with some RFC describing:

• What you want to achieve, what is the scope of your change
• Directions considered
• Pros and cons of used solution
• Known limitations

[elhewaty]

don't you think we need to implement the hoisting pass for similar cases mentioned here #194 (comment) if not what do you suggest?

[asl]

don't you think we need to implement the hoisting pass for similar cases mentioned here #194 (comment) if not what do you suggest?

Why this is needed? What problems will it solve? This is exactly why you'd need to start from RFC.

[elhewaty]

I think I don't need the renaming, and hoisting pass, as it is SSA form after all, so all vars in the same states are unique. but we need to move only Declarations in the top of the state that have uses after the IfOp, and also we need to create new states for the Then and Else regions.

state a {}
state a_true {}
state a_false {}
state a_merge{
if ...
else ...
}
remove this 
It will be 
state a_merge_true {}
state a_merge_false {}
...
And so on

regarding the variable names: as all states in the same scope are unique so for the state vars

var name = state_name + var_name

// global scope
var a_x = ...
state a {}

There will be a name conflict if there is already a state with same name as the resulted names.
But this is in the state names only. As states starts with @ so they cannot conflict vars. One solution is appending a number suffex after the state name that is unique to each state with the same name. But is this good enough at very big states, in the worst case the number will overflow, maybe there is a restriction in the language itself to prevent this?

[asl]

Looks like you're making simple things too complex. Also for some reason you're not following P4HIR semantics. So let me summarize a bit:

1. Variable names (name attribute of p4hir.variable op) are optional. While it is great that they won't clash, it has not further semantical meaning. The variable names are neither externally visible nor play any role in the IR (in contrast with the P4C internal IR which does rely on variable names and has to run over multiple hoops and complications enforced by this).
2. What matters is the original source code location. And we have to preserve it while doing any transformations.
3. Technically there is no need to create new variables. One needs to move only those p4hir.variable ops that have uses in the "bottom" part of the state. Certainly, one of the easiest way to move is just to clone and do a RAUW. But still it does not require any complicated things, new passes, etc.
4. Symbol names (states themselves) are required to be unique within a symbol table. Still, state names are not control-plane visible, so we can rename them however we want. Even more, we can ask symbol table to generate a unique name for us. There are already examples in the translator how this could be done (around overload sets handling) and we should just use similar mechanism rather than inventing a wheel.

Oh, yes, and pass should handle switches and ternary ops as well. Though it is pretty straightforward as soon as the majority of the code is in place.

[elhewaty]

@asl, It doesn't handle switch yet. but I thought it is a good idea to show you where did I reach.

input

#false = #p4hir.bool<false> : !p4hir.bool
#true = #p4hir.bool<true> : !p4hir.bool
module {
  p4hir.parser @simple_parser(%arg0: !p4hir.ref<!p4hir.bool>, %arg1: !p4hir.ref<!p4hir.bool>)() {
    p4hir.state @start {
      %false = p4hir.const #false
      p4hir.assign %false, %arg1 : <!p4hir.bool>
      %condition = p4hir.read %arg1 : <!p4hir.bool>
      %bla = p4hir.read %arg0 : <!p4hir.bool>
      p4hir.if %condition {
        p4hir.assign %condition, %arg1 : <!p4hir.bool>
      } else {
        p4hir.assign %bla, %arg0 : <!p4hir.bool>
      }
      p4hir.transition to @simple_parser::@final_state
    }
    p4hir.state @final_state {
      // different scope, so different condition 
      %condition = p4hir.read %arg1 : <!p4hir.bool>
      p4hir.assign %condition, %arg0 : <!p4hir.bool>
      p4hir.parser_accept
    }
    p4hir.transition to @simple_parser::@start
  }
}

output

#false = #p4hir.bool<false> : !p4hir.bool
#true = #p4hir.bool<true> : !p4hir.bool
module {
  %true = p4hir.const #true
  %false = p4hir.const #false
  p4hir.parser @simple_parser(%arg0: !p4hir.ref<!p4hir.bool>, %arg1: !p4hir.ref<!p4hir.bool>)() {
    %val = p4hir.read %arg0 : <!p4hir.bool>
    %val_0 = p4hir.read %arg1 : <!p4hir.bool>
    p4hir.state @start {
      p4hir.assign %false, %arg1 : <!p4hir.bool>
      p4hir.transition_select %val_0 : !p4hir.bool {
        p4hir.select_case {
          p4hir.yield %true : !p4hir.bool
        } to @simple_parser::@start_if_true_0
        p4hir.select_case {
          p4hir.yield %true : !p4hir.bool
        } to @simple_parser::@start_if_true_0
      }
    }
    p4hir.state @start_if_false_0 annotations {} {
      p4hir.assign %val, %arg0 : <!p4hir.bool>
      p4hir.transition to @simple_parser::@start_if_join_0
    }
    p4hir.state @start_if_true_0 annotations {} {
      p4hir.assign %val_0, %arg1 : <!p4hir.bool>
      p4hir.transition to @simple_parser::@start_if_join_0
    }
    p4hir.state @start_if_join_0 annotations {} {
      p4hir.transition to @simple_parser::@final_state
    }
    p4hir.state @final_state {
      %val_1 = p4hir.read %arg1 : <!p4hir.bool>
      p4hir.assign %val_1, %arg0 : <!p4hir.bool>
      p4hir.parser_accept
    }
    p4hir.transition to @simple_parser::@start
  }
}

[elhewaty]

Ah, sorry I passed true value for the false state, too. I will modify it tomorrow, it is 1 am here. But is the logic correct?

[asl]

        p4hir.select_case {
          p4hir.yield %true : !p4hir.bool
        } to @simple_parser::@start_if_true_0

Select cases are expected to yield sets. We have TBD for the verifier here.

So, it should be:

        p4hir.select_case {
          %set = p4hir.set (%true) : !p4hir.set<!p4hir.bool>
          p4hir.yield %set : !p4hir.set<!p4hir.bool> 
        } to @simple_parser::@start_if_true_0