Page MenuHomePhabricator

[fir] Add FIR CSE specific pass
AbandonedPublic

Authored by clementval on Oct 28 2021, 5:37 AM.

Details

Summary

Perform common subexpression elimination on FIR operations. This pass
differs from the MLIR CSE pass in that it is FIR/Fortran semantics aware.

It takes advantages of additional Fortran semantic such as pure function call.
pure procedures and functions are free of side effects. In some cases,
pure calls can be safely removed.

fir.load are also optimized by this dedicated CSE pass. Volatile loads will
prevent the optimization to happen.

Running the same test (flang/test/Fir/cse.fir) through the MLIR CSE
pass will not give a similar results.

This patch is part of the upstreaming effort from fir-dev branch.

Diff Detail

Event Timeline

clementval created this revision.Oct 28 2021, 5:37 AM
clementval requested review of this revision.Oct 28 2021, 5:37 AM
flang/test/Fir/cse.fir
31

Not asking for a change here. I made the following change and it retains a fir.call in the second block. I guess this is because we process blocks separately.

-  %9 = arith.addi %7, %8 : i64
+  br ^bb2
+ ^bb2:  %9 = arith.addi %7, %8 : i64
37

I think COUNT will not check that the addis are consecutive with no fir.calls in between.
See test in the comment above.

schweitz accepted this revision.Oct 28 2021, 3:16 PM
This revision is now accepted and ready to land.Oct 28 2021, 3:16 PM
mehdi_amini requested changes to this revision.Oct 28 2021, 4:23 PM

This pass differs from the MLIR CSE pass in that it is FIR/Fortran semantics aware.

Can you expand and provide details about this?

(marking as "request changes" because @clementval told me to do so when discussing a revision, I haven't looked at the code at this point)

This revision now requires changes to proceed.Oct 28 2021, 4:23 PM
  • split input file for the test
  • make the checks more explicit
clementval edited the summary of this revision. (Show Details)Oct 29 2021, 2:06 AM

This pass differs from the MLIR CSE pass in that it is FIR/Fortran semantics aware.

Can you expand and provide details about this?

(marking as "request changes" because @clementval told me to do so when discussing a revision, I haven't looked at the code at this point)

I updated the description about some information. This pass mainly use semantic information on fir.call and fir.load and optimized them out when possible. This is relying on the pure semantic for the calls.
The optimization on fir.load will be prevented if they are flagged as volatile loads.

I think we should use "request changes" if there is any blocker change requested in the code. I guess this is also fair usage if you want to block this patch for discussion.

Right so I'm interested in what it'll take to converge this with the existing CSE pass. What you mentioned here is interesting because it seems all in the domain of memory effects I believe: does it point mostly to the lack of support for the memory effects op interface in the CSE pass?

Right so I'm interested in what it'll take to converge this with the existing CSE pass. What you mentioned here is interesting because it seems all in the domain of memory effects I believe: does it point mostly to the lack of support for the memory effects op interface in the CSE pass?

I think what difference we have right now with the memory effects op interface is that in the case of the memory effects op interface it applies to a op all the time. In our case, the fir.call can be pure or not. I'm not sure how this would work with the memory effects. It would probably requires two distincts operations or I'm missing something. Sorry, I'm not be up to date with the current memory effects op interface (will have a look at it today to refresh my knowledge :-) ).

Right so I'm interested in what it'll take to converge this with the existing CSE pass. What you mentioned here is interesting because it seems all in the domain of memory effects I believe: does it point mostly to the lack of support for the memory effects op interface in the CSE pass?

I think what difference we have right now with the memory effects op interface is that in the case of the memory effects op interface it applies to a op all the time. In our case, the fir.call can be pure or not. I'm not sure how this would work with the memory effects.

When an an operation implements the interface, it can write any kind of C++, including looking up attributes on the operation. This is very much a query made on a specific instance of the operation and not a query that has to be static on the op class I believe.

See for example:

Right so I'm interested in what it'll take to converge this with the existing CSE pass. What you mentioned here is interesting because it seems all in the domain of memory effects I believe: does it point mostly to the lack of support for the memory effects op interface in the CSE pass?

I think what difference we have right now with the memory effects op interface is that in the case of the memory effects op interface it applies to a op all the time. In our case, the fir.call can be pure or not. I'm not sure how this would work with the memory effects.

When an an operation implements the interface, it can write any kind of C++, including looking up attributes on the operation. This is very much a query made on a specific instance of the operation and not a query that has to be static on the op class I believe.

See for example:

Thanks for the pointers. I'll have a look at that.

Coming back to this. Looks like the existing CSE is not able to deal with simple use case without analysis. Given the example below I would expect the pass to remove one of the redundant load but this is not happening with the existing MLIR CSE pass but we have such feature in the FIR CSE pass. Maybe I'm missing something but I guess that without a proper analysis the MLIR CSE pass would not be able to deal with such cases in FIR. Should we go ahead and have our dedicated CSE for specific FIR operations or is there smth I'm missing?

func @fun(%arg0: !fir.ref<i64>) -> i64 {
    %0 = fir.load %arg0 : !fir.ref<i64>
    %1 = fir.load %arg0 : !fir.ref<i64>
    %2 = arith.addi %0, %1 : i64
    return %2 : i64
}
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2022, 12:36 PM

I think it would be more productive as a community to improve the existing CSE to figure out how to make it work for these cases instead.

I think it would be more productive as a community to improve the existing CSE to figure out how to make it work for these cases instead.

Sure. I'll look how we can make it work.

First patch in this direction here: https://reviews.llvm.org/D122801

clementval abandoned this revision.Jun 14 2022, 10:21 AM