This is an archive of the discontinued LLVM Phabricator instance.

[flang] Introduce `AbstractResultOnGlobalOpt` pass
ClosedPublic

Authored by unterumarmung on Jul 19 2022, 7:13 AM.

Details

Summary

This pass allows to convert operations
which use functions with abstract results to ones that do not.

Depends on D130087

Diff Detail

Event Timeline

unterumarmung created this revision.Jul 19 2022, 7:13 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
unterumarmung requested review of this revision.Jul 19 2022, 7:13 AM

Rebased & updated tests

I wonder if there's a chance that there'll be a function type with abstract results inside fir.global type. Is it possible? If so, how should it be handled?

I wonder if there's a chance that there'll be a function type with abstract results inside fir.global type. Is it possible? If so, how should it be handled?

That is a good point, I think it will probably happen once procedure pointers are lowered, and if the related function types are represented (currently, for dummy procedures, we always cast the function type before calls to the interface know at the call site).

In my opinion, it is fine if you leave a TODO for this case until we know more how we want to deal with procedure pointers (one difficulty I foresee if we fully type them, if how we will deal with derived type rewrites for function pointer components. I am not sure how one can replace a named type in MLIR.).

I wonder if there's a chance that there'll be a function type with abstract results inside fir.global type. Is it possible? If so, how should it be handled?

That is a good point, I think it will probably happen once procedure pointers are lowered, and if the related function types are represented (currently, for dummy procedures, we always cast the function type before calls to the interface know at the call site).

In my opinion, it is fine if you leave a TODO for this case until we know more how we want to deal with procedure pointers (one difficulty I foresee if we fully type them, if how we will deal with derived type rewrites for function pointer components. I am not sure how one can replace a named type in MLIR.).

Ok! What is the best way to do this?

  1. For a simple procedure pointer this expression probably is not enough. because I think procedure pointers will have FunctionType nested like !fir.ptr<() ->()> or !fir.boxproc<() -> ()>, so I need to iterate depth-wise
global.getType().cast<FunctionType>()
  1. For the procedure components the situation is more interesting. For the program in this comment, if I comment all of the TODO(..., "support for polymorphic types"), I get this code. It's just lowering, without optimization and transformation. I can't quite understand what is happening there and how I should handle it.

Any help is appreciated, thanks!

I wonder if there's a chance that there'll be a function type with abstract results inside fir.global type. Is it possible? If so, how should it be handled?

That is a good point, I think it will probably happen once procedure pointers are lowered, and if the related function types are represented (currently, for dummy procedures, we always cast the function type before calls to the interface know at the call site).

In my opinion, it is fine if you leave a TODO for this case until we know more how we want to deal with procedure pointers (one difficulty I foresee if we fully type them, if how we will deal with derived type rewrites for function pointer components. I am not sure how one can replace a named type in MLIR.).

Ok! What is the best way to do this?

  1. For a simple procedure pointer this expression probably is not enough. because I think procedure pointers will have FunctionType nested like !fir.ptr<() ->()> or !fir.boxproc<() -> ()>, so I need to iterate depth-wise

Yes, although I do not know yet if we will lower the function type to something else than () -> () in the procedure pointer types or if we will keep relying on function type cast on the call site. Rewriting types, especially in nested structures involving named types does not look like a fun journey, so we may want to keep the types as () -> (), we should weight the pros and cons when dealing with procedure pointers.

Right now you could probably check if the global type needs to be rewritten (if it contains a nested function type), in which case you could emit a TODO(loc, ....).

  1. For the procedure components the situation is more interesting. For the program in this comment, if I comment all of the TODO(..., "support for polymorphic types"), I get this code. It's just lowering, without optimization and transformation. I can't quite understand what is happening there and how I should handle it.

I do not think there is anything required here on top of what you have done. The issue with procedure components was that:

  • These data structures are describing the procedure components so that the runtime can resolve calls involving CLASS(T). They are using the ISO C c_funptr type to hold the pointers, and this type erase the function types in the data structure. However, the FIR operations inserting the actual procedure component address into the fir.global are dealing with the actual function type:
%3 = fir.address_of(@_QMaPfoo) : (!fir.box<!fir.type<_QMaTf>>) -> !fir.box<!fir.heap<!fir.char<1,?>>>
 %c-1 = arith.constant -1 : index
 %4 = fir.field_index __address, !fir.type<_QM__fortran_builtinsT__builtin_c_funptr{__address:i64}>
 %5 = fir.convert %3 : ((!fir.box<!fir.type<_QMaTf>>) -> !fir.box<!fir.heap<!fir.char<1,?>>>) -> i64
 %6 = fir.undefined !fir.type<_QM__fortran_builtinsT__builtin_c_funptr{__address:i64}>
 %7 = fir.insert_value %6, %5, ["__address", !fir.type<_QM__fortran_builtinsT__builtin_c_funptr{__address:i64}>] : (!fir.type<_QM__fortran_builtinsT__builtin_c_funptr{__address:i64}>, i64) -> !fir.type<_QM__fortran_builtinsT__builtin_c_funptr{__address:i64}>
  ....

Hence, the fir.address_of inside the GlobalOp needed to be rewritten if it dealt with functions returning arrays or descriptors, which is what your patch should enable.

Note that procedure components are not represented in FIR fir.type derived types because they are not data objects. So There is no rewrite needed in the related fir.type.

jeanPerier added inline comments.Jul 27 2022, 5:56 AM
flang/include/flang/Optimizer/Transforms/Passes.h
13

Small style question, it looks like clang-format is happy with your change, but is it from MLIR/LLVM style to add blank lines between the headers from different parts ? I am not sure we are doing it in other files.

flang/test/Fir/abstract-results.fir
320

The cases with alloca/calls inside global op are a bit unrealistic: at least at the LLVM dialect level that is forbidden, the region should be constant foldable to a compile time constant. Now, given we do not enforce this in the fir.global verifier, I am not completely against having these tests (in a far fetch future we could imagine doing some mandatory inlining of "constexpr" functions + mem to reg in the fir global op bodies, but that is very unlikely).

What is the motivating Fortran example here?

flang/include/flang/Optimizer/Transforms/Passes.h
13

Don't use blank lines.

flang/lib/Optimizer/Transforms/AbstractResult.cpp
313

The name of this pass appears to have nothing to do with the description.

What is really going on?

flang/test/Fir/abstract-results.fir
320

Agreed.

I suspect this isn't a necessary extension to FIR. Is there an actual example of Fortran code that requires this?

What is the motivating Fortran example here?

The motivating example code is here: https://reviews.llvm.org/D130369#3672155
I get a TODO error I removed in the patch if I remove all of TODO(..., "support for polymorphic types") first.

I decided to implement this because there have been code written for FuncOp, so all I needed to do is to refactor some things and handling of the GlobalOp.

flang/include/flang/Optimizer/Transforms/Passes.h
13

Ok

flang/lib/Optimizer/Transforms/AbstractResult.cpp
313

This pass converts operations that

  1. Use a function type with an abstract result
  2. Are inside of a fir::GlobalOp

to an equivalent instruction without the abstract result.

313

How should I rename the pass?

flang/test/Fir/abstract-results.fir
320

There is no motivating example for this test particularly.
I just adapted as much of the FuncOp tests as possible like a proof of concept.

So, I should remove all of GlobalOp tests which contain fir.alloca and any call instructions?

@jeanPerier, thank you for the detailed reply, It has been really helpful!

Yes, although I do not know yet if we will lower the function type to something else than () -> () in the procedure pointer types or if we will keep relying on function type cast on the call site. Rewriting types, especially in nested structures involving named types does not look like a fun journey, so we may want to keep the types as () -> (), we should weight the pros and cons when dealing with procedure pointers.

Ok! But I got stuck on this.
What I think the best solution is traversing all of the nested types of the given type of GlobalOp until we find a function type with an abstract type or don't find it at all.
But fir::GlobalOp::getType() returns a mlir::Type object. It doesn't have any methods to return a list of type parameters. Is there a way to get it? Could you please give some tips?

Note that procedure components are not represented in FIR fir.type derived types because they are not data objects. So There is no rewrite needed in the related fir.type.

Interesting, so what I've been trying to do here is completely wrong..

Rebased & added TODO

@jeanPerier, thank you for the detailed reply, It has been really helpful!

Yes, although I do not know yet if we will lower the function type to something else than () -> () in the procedure pointer types or if we will keep relying on function type cast on the call site. Rewriting types, especially in nested structures involving named types does not look like a fun journey, so we may want to keep the types as () -> (), we should weight the pros and cons when dealing with procedure pointers.

Ok! But I got stuck on this.
What I think the best solution is traversing all of the nested types of the given type of GlobalOp until we find a function type with an abstract type or don't find it at all.
But fir::GlobalOp::getType() returns a mlir::Type object. It doesn't have any methods to return a list of type parameters. Is there a way to get it? Could you please give some tips?

The closest thing that will give you what you want is the SubElement interfaces: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/SubElementInterfaces.td

They provide API for walking sub attributes/types.

schweitz added inline comments.Aug 3 2022, 11:04 AM
flang/lib/Optimizer/Transforms/AbstractResult.cpp
313

Maybe something that conveys "transformation of calls that return abstract results in a global". The name suggested to me that it was rewriting globals to have abstract types. Which seems quite odd.

flang/lib/Optimizer/Transforms/PassDetail.h
13

nit: blank line

flang/test/Fir/abstract-results.fir
269

I think the compiler should simply ban the use of alloca and allocmem in GlobalOp.

A GlobalOp is expected to have no resources and not be able to acquire any resources. So memory allocation, function calls, etc. just do not make much sense in a GlobalOp. More precisely, a GlobalOp is expected to become bits in a .data, .rodata, or .bss section of the object file. It's "code" is meant to construct an initial value at compile-time only—it's body should never, in practice, become executable code.

Given that, it doesn't seem like this pass is needed.

unterumarmung added inline comments.Aug 3 2022, 11:14 AM
flang/test/Fir/abstract-results.fir
269

I see your point.
But this is the easiest way to "solve" this TODO: https://reviews.llvm.org/D130369, that's why I implemented it like this.
What is the best/expected way to do this?

@jeanPerier, thank you for the detailed reply, It has been really helpful!

Yes, although I do not know yet if we will lower the function type to something else than () -> () in the procedure pointer types or if we will keep relying on function type cast on the call site. Rewriting types, especially in nested structures involving named types does not look like a fun journey, so we may want to keep the types as () -> (), we should weight the pros and cons when dealing with procedure pointers.

Ok! But I got stuck on this.
What I think the best solution is traversing all of the nested types of the given type of GlobalOp until we find a function type with an abstract type or don't find it at all.
But fir::GlobalOp::getType() returns a mlir::Type object. It doesn't have any methods to return a list of type parameters. Is there a way to get it? Could you please give some tips?

The closest thing that will give you what you want is the SubElement interfaces: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/SubElementInterfaces.td

They provide API for walking sub attributes/types.

Yes, but to use this, I need to change the definitions of the types I want to iterate through, which is overkill in this particular situation.
Thank you for the information, I'll keep this in mind!

The closest thing that will give you what you want is the SubElement interfaces: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/SubElementInterfaces.td

Thanks for pointing that out @rriddle, we will definitely want to use this in flang to ease all the passes that need to visit types, and to get nice type aliasing when printing nested types.

Yes, but to use this, I need to change the definitions of the types I want to iterate through, which is overkill in this particular situation.
Thank you for the information, I'll keep this in mind!

I agree that adding the generic visit is outside of the scope of your patch. Your "containsFunctionTypeWithAbstractResult" function does the job of making to insert the TODO, we will take it when it is clear of Procedure pointer types should be represented.

flang/test/Fir/abstract-results.fir
269

@schweitz, the pass is needed to rewrite the fir::AddrOfOp to function inside GlobalOp, which are legal and makes sense. Because the pass re-uses the FuncOp pass logic, it comes with patterns for CallOp and such: we simply do not care about those in the context of a GlobalOp, we should not have these operation in a GlobalOp.

The GloablOp pass could also just share the fir::AddrOfOp registration and pattern logic to avoid being misleading. The downside being that if some new Operation that may refer to a function symbol is used and can be used in both GlobalOp and FuncOp, one would have to not forget adding it to both passes. With the current approach, they would be registered for all passes, and just unused if it is not possible to have the related op in a GloablOp anyway.

320

So, I should remove all of GlobalOp tests which contain fir.alloca and any call instructions?

Yes, I think it may mislead people using tests as an example of what FIR canonical patterns are.

schweitz added inline comments.Aug 4 2022, 9:09 AM
flang/test/Fir/abstract-results.fir
269

Thanks, Jean.

100% agree that all function types should be rewritten to correspond appropriately to the low-level target and that includes any (legal) addr_of Ops in a GlobalOp.

  1. Rebase
  2. Remove blank lines
  3. Remove non-conforming tests
unterumarmung marked 2 inline comments as done and an inline comment as not done.Aug 9 2022, 4:30 AM

Hey, @jeanPerier @schweitz! Sorry for the delay

I rebased my patch and addressed most of the review comment. Could you please take a look?

flang/lib/Optimizer/Transforms/AbstractResult.cpp
313

Maybe, AbstractResultInGlobalOpt? :)

jeanPerier accepted this revision.Aug 23 2022, 12:01 AM

Thanks for addressing the comments, LGTM

This revision is now accepted and ready to land.Aug 23 2022, 12:01 AM
This revision was landed with ongoing or failed builds.Aug 25 2022, 12:00 PM
This revision was automatically updated to reflect the committed changes.