tensorflow / mlir-hlo

398 stars 70 forks source link

while loop simplifier missing #34

Closed lipracer closed 1 year ago

lipracer commented 2 years ago

xla service has while_loop_simplifier. Invariant's code Motion function MHLO already exists. But the following optimizations are missing.

Currently MHLIR ::WhileOp just lower to SCF ::WhileOp, I'm not sure if SCF ::WhileOp has a similar simplification. If MLIR ::WhileOp needs to support this functionality, it may need to simulator excute 'whileOp''s body, or use data flow analysis to implement constant propagation to simulate body execution of 'whileOp.

Mogball commented 2 years ago

I recently changed scf.while to be simplifiable by SCCP, after fixing a few bugs with getRegionSuccessors and how that function was getting used. The same can be done for mhlo.while.

lipracer commented 2 years ago

Thanks a lot, I will make a reference and then implement the simplification of mhlo::while.

joker-eph commented 2 years ago

Isn't a folder for the WhileOp when the return of the condition is false enough here?

lipracer commented 2 years ago

Yeah.First we need to propagate the constant to cond's body, then try to fold it's.

while(%cst0, %cst1) {
  cond (%arg0, %arg1) {
    return compare(%arg0, %arg1)  
  }
}

and the mhlo-sink-constants-to-control-flow just clone constantOp into body, didn't clone arguments.

joker-eph commented 2 years ago

I have a patch for the folder, but I also looked into the RegionBranchOpInterface and it seems like there is a limitation to the interface that prevents us from using it in MHLO.

In particular the getMutableSuccessorOperands for RegionBranchTerminatorOpInterface can't be used with our while op because the condition terminator does not take as operands the values to pass to the body...

lipracer commented 2 years ago

Can we apply the framework of data flow analysis, implemented by constant propagation? Including other control flow simplifications, I'm not sure if scf dailect does these mhlo's as well, but we need these for completeness.

joker-eph commented 2 years ago

This relies on RegionBranchOpInterface, which is difficult to apply here (what I'm describing above)

Mogball commented 2 years ago

In particular the getMutableSuccessorOperands for RegionBranchTerminatorOpInterface can't be used with our while op because the condition terminator does not take as operands the values to pass to the body...

I've run into this limitation before but just added as operands the forwarded values. I will check whether the function really needs to return a MutableOperandRange or whether it can just return a ValueRange. Then, the terminator could just return the block arguments.

joker-eph commented 2 years ago

Thanks!

lipracer commented 2 years ago

Thanks.I need to know about RegionBranchOpInterface. But xla service already has while_loop_simplifier.

Mogball commented 2 years ago

I will check whether the function really needs to return a MutableOperandRange or whether it can just return a ValueRange

The only upstream use of the MutableOperandRange return type is in buffer deallocation.

This signature matches with the interface for BranchOpInterface in that it allows transformations to reason and modify control-flow constructs. I'm not sure it can go away.