[CIR] Introduce CIR simplification

[gitoleg]

This PR introduce cir simplification pass. The idea is to have a pass for the redundant operations removal/update.

Right now two pattern implemented, both related to the redundant bool operations.
First pattern removes redundant casts from bool to int and back that for some reasons appear in the code.
Second pattern removes sequential unary not operations (!!) .

For example, the code from the test is expanded from the simple check that is common for C code:

#define CHECK_PTR(ptr)  \
  do {                   \
    if (__builtin_expect((!!((ptr) == 0)), 0))\
      return -42; \
  } while(0)

I mark this PR as a draft for the following reasons:

1. I have no idea if it's useful for the community
2. There is a test fail - unfortunately current pattern rewriter run DCE underneath the hood and looks like we can't workaround it.
   It's enough just to add an operation to the list - in this case UnaryOp - and call applyOpPatternsAndFold. I could rewrite a test a little in order to make everything non dead or implement a simple fix point algorithm for the particular task (I would do the former).

[bcardosolopes]

Sorry I didn't had the time to look just yet, but should do that soon, can you update post rebase?

[gitoleg]

@bcardosolopes updated!

[bcardosolopes]

This PR introduce cir simplification pass. The idea is to have a pass for the redundant operations removal/update.

Right now two pattern implemented, both related to the redundant bool operations. First pattern removes redundant casts from bool to int and back that for some reasons appear in the code. Second pattern removes sequential unary not operations (!!) .

Neat!

1. I have no idea if it's useful for the community

Definetely great to get rid of these chains of redundant casts.

2. There is a test fail - unfortunately current pattern rewriter run DCE underneath the hood and looks like we can't workaround it.

Is this something related to the same issues we see with the canonicalizer? Is it worth adding this case to the bug tracking it?

It's enough just to add an operation to the list - in this case UnaryOp - and call applyOpPatternsAndFold. I could rewrite a test a little in order to make everything non dead or implement a simple fix point algorithm for the particular task (I would do the former).

Former is a fine solution, I'm a bit worried about the overall approach though (more comments inline), I think we should try as much as possible to fix some of these problems at the MLIR level if necessary, I'm afraid those will keep piling up and might accumlate be too much technical debt in the future (e.g. CIR reinventing the wheel to workaround MLIR issues). Thoughts?

[bcardosolopes]

Any specific reason why can't this be implemented in CastOp::fold?

[bcardosolopes]

Similar question for UnaryOp, can't that be done with fold method?

[bcardosolopes]

If we need to create a new pass (after we go over the other comments), perhaps this is a good opportunity to rename MergeCleanups to CIRSimplify and add this over there instead.

[gitoleg]

@bcardosolopes
Frankly, I don't know if it's possible to implement it as fold, since we need to erase operations and return values of different types - I tried and failed :( For example, for the CastOp::fold I returned %6

%7 = cir.cast(bool_to_int, %6 : !cir.bool), !cir.int<s, 32>
%8 = cir.cast(int_to_bool, %7 : !cir.int<s, 32>), !cir.bool

but anyway got a type error. Maybe I'm doing something wrong though and didn't get how I should work wtih folders.

Another thought is that from the future optimizations point of view, I would have CIR simplified as much as we can do it, and better to do it explicit with the pass, than implicit with fold which is called somewhere. Well, maybe I'm wrong.

Is this something related to the same issues we see with the canonicalizer? Is it worth adding this case to the bug tracking it?

I would say it's not a bug, it's a feature!! which may hit us only in tests. It's slightly remind us the case with the canonicalizer since it also run the version of applyPatternsAndFold underneath of the hood. I don't think we need to create another issue here.

To summarize:

1. I would create a separate pass to handle simplifications. But I still need your advice in case I missed something with folders.
2. I fixed tests - just add "use" of operations.

[bcardosolopes]

For example, for the CastOp::fold I returned %6

%7 = cir.cast(bool_to_int, %6 : !cir.bool), !cir.int<s, 32>
%8 = cir.cast(int_to_bool, %7 : !cir.int<s, 32>), !cir.bool

From the canonicalization docs:

/// 3. They can return an existing value or attribute that can be used instead
/// of the operation. The caller will remove the operation and use that
/// result instead.

In this case sounds like it wouldn't be a problem for the folder for cir.cast(int_to_bool, to return %6 directly? The folder would have to look at the defining op for %7 and check it's a bool_to_int. Given the folder expects bool to be returned, looks like %6 would match?

Another thought is that from the future optimizations point of view, I would have CIR simplified as much as we can do it, and better to do it explicit with the pass, than implicit with fold which is called somewhere. Well, maybe I'm wrong.

Simplifications might work better in face of canonicalized IR, but it's a blurry line sometimes where things should live. In the cir.cast(int_to_bool, case, it feels more like canonicalization. But overall if it's doing too much (deleting ops, blocks, adding new ops, etc), I agree with you.

1. I would create a separate pass to handle simplifications. But I still need your advice in case I missed something with folders.

MergeCleanups is not about folding, turns out that some of the easier things we moved into folders. I'd prefer we rename it to CIRSimplify and add transformations there as well.

My overall feedback is that I want to see the same optimizations you are trying to implement. But my feedback for this PR is that such simplification is doing too much in one place, pieces should be more independent, behave more like traditional
optimizations or canonicalization. For example: removing redundant casts is overall goodness that shouldn't be depending on optimizing unarys.

[bcardosolopes]

What's the C code corresponding to this? Add a C code test instead with what you want to optimize.

For a cir-opt tests, better to split into two really simple CIR functions, each one testing the specific opt, so it's pretty clear you are adding coverage for different individual peepholes.

[bcardosolopes]

Do you need to change the source here because otherwise these operations are removed? Does everything else in the function touching f gets removed or just this one? Perhaps the function should return a float instead so that the value is used? If simplification is getting too aggresive, perhaps we should instead tie it up with O1 or higher, because -O0 shouldn't delete as much operations. Example: https://godbolt.org/z/6zP7qnjdP

[bcardosolopes]

The opt logic seems not to take into account integer promotion happening with casts here, which could be unsafe. Depending on the pattern you are trying to get rid off, it might be better to make sure CIRGen gets it right out of the bat.