CSE Pass can not eliminate two Ops with the same region

MLIR
1

Hi all,
When using CSE pass, I found that if an Op has Region attribute， CSE Pass will just look into its region, and do CSE on the ops in its region. But actually, if two ops has the same region( which means all ops in their region are same and has no side effect), these two ops should also be treated as common sub-expression and should keep only one of them.
I think it is necessary to provide a comparison function between Op’s that own regions, and do CSE among them.

2

Hi @Gxiandy . Could you post some IR… CSE currently only handles ops with a single region and single block. Its a TODO to handle more complex regions (also not sure if that is a good thing to do). But if you post the IR I can see whats happening.

3

Hi @MaheshRavishankar . For example, we have the following IR

func.func @cse(%arg0: memref<12x64x64x64xsi8>) -> memref<12x57x57x64xsi8> {
%0 = "const"() {values = dense<3> : tensor<64x2xsi64>} : () -> memref<64x2xsi64> // bias
%1 = "const"() {values = dense<2> : tensor<64x8x8x1xsi8>} : () -> memref<64x8x8x1xsi8>
%2 = "conv2d"(%arg0, %1, %0) ({}) {} : (memref<12x64x64x64xsi8>, memref<64x8x8x1xsi8>, memref<64x2xsi64>) -> memref<12x57x57x64xsi8>
%3 = "conv2d"(%arg0, %1, %0) ({}) {} : (memref<12x64x64x64xsi8>, memref<64x8x8x1xsi8>, memref<64x2xsi64>) -> memref<12x57x57x64xsi8>
%4 = "eltwise"(%2, %3) ({
  }) {} : (memref<12x57x57x64xsi8>, memref<12x57x57x64xsi8>) -> memref<12x57x57x64xsi8>
return %4 : memref<12x57x57x64xsi8>
}

These two conv2d Ops will have an empty region(which will be used when lower these op to assembly code), so they will be not able to be estimated.

In fact， these two Ops are identical because they have the same functionality and attributes，and also have the same region(which are empty).

4

Are these conv2d ops defined as having no side effects?

5

Yes, they are defined as no side effects

6

You may want to ensure the dialect that they were part of was registered on the tool that you used to run -cse. I notice that your snippet is in generic form: so the parsing will always succeed (even if the dialect wasn’t registered).

7

Note that this is an opt-in (you need to pass in --allow-unregistered-dialects), and the “gotcha” you’re pointing at is why I discourage using this option as much as possible! It’s quite a footgun…

8

In my test，these conv2d Ops are in a dialect which has been registered. I removed the dialect prefix when posting this snippet.

9

Ha! Looks like just a missed case where the region is empty. Do you mind filing an issue on LLVM GitHub with MLIR label?

10

I’d like to do that. In fact, I think not only the empty region case has been missed, the Ops with the same region and have no side effect should also be treated as common sub-expressions.
But there’s one thing I’m not sure about. Let’s say we have two block Ops with the same operation list in their region, and all of the operations have no side effect. If we treat ops with the same region as common sub-expression, then we will keep only one of the blocks. I am not sure if removing one of the block will cause new problems.

11

As I said above that ops with a single region and single block should be CSEd already. So filing an issue and following up on that would be the best way forward. I am happy to take a look

12

Hi, @MaheshRavishankar. I have pushed my local patch here, and I am working on adding testcases for it. Please let me know if there is anything wrong.