Converting abstract results of functions inside `fir::GlobalOp`

Hi!

I recently stumbled upon this codegen TODO:

And I started to explore ways to fix it.

This TODO (in my case) is fired when there some operations like fir.address_of use procedures with non-trivial return types like dynamically shaped arrays which are referred as abstract results.

Then I found out that flang already has pass that converts such abstract results to be an out-parameter in a function:

I tried to generalize the pass in this patch but I have some problems with it - old tests for some reason stopped working:

Interestingly enough, there are some other tests that check conversion of the abstract results, like this one, and they didn’t fail. It seems like I miss something simple and obvious.

I’d like to fix the issue and upstream it, so any help is appreciated!

CC: @kiranchandramohan @clementval

Can you post your change as a phab review with [WIP] in the title? It will be easier to see the differences in code and help you from there.

I’ve already done that: ⚙ D129199 WIP: abstract results on GlobalOp. Maybe the link wasn’t obvious in the post.

Or you want me to make it non-draft review with [WIP] prefix in the title?

Sorry, I didn’t see that. I’ll have a look at this.

1 Like

Thanks!

@clementval Sorry for ignoring, I totally forgot that in “draft” revisions there are no notifications to email and I was relying on them…

As you suggested, I converted AbstractResultsOpt pass to run on ModuleOp:
https://reviews.llvm.org/D129486
It doesn’t work either for some reason

Also, I propose a little refactoring for the pass:
https://reviews.llvm.org/D129485

I’m not sure what doesn’t work. Shouldn’t you have a conversion pattern for GlobalOp in the pass? I don’t see any.

I didn’t add it for the moment. I just wanted to make the refactoring first. Adding logic for Global Op should be easy after the refactoring is done.

Also, I think we should not add something for the GlobalOp, because GlobalOp itself shouldn’t be changed, only operations inside it.

@clementval, I got this working. Can you review it please? I want to make this easy to review, so I created 2 patches:

  1. ⚙ D129485 [flang][NFC] Drop `AbstractResultOptions` structure - simple refactoring
  2. ⚙ D129486 [flang] Make `AbstractResultsOpt` run on `ModuleOp`

The next step will be to add logic for fir::GlobalOp. I assume that it works already - just need to add tests.

@clementval
I submitted another patch: ⚙ D129778 [flang] Generalize `AbstractResultOpt` pass that implements what you described in this ⚙ D129486 [flang] Make `AbstractResultsOpt` run on `ModuleOp` comment

Thanks for the patches. I’ll try to have a look at it before next week.

1 Like

I submitted 2 more patches:

  1. Just NFC for tests, so they will be easier to read: ⚙ D130087 [flang][NFC] Unify check prefixes in `abstract-results.fir` test
  2. As you suggested, I created a patch for GlobalOp conversion: ⚙ D130088 [flang] Introduce `AbstractResultOnGlobalOpt` pass. In my humble opinion, the new pass fits pretty nicely into the existing code.

Hi, @clementval!

Should I wait on your review on this patch: ⚙ D129778 [flang] Generalize `AbstractResultOpt` pass or I can merge it since @jeanPerier approved it?

I added some comments. You can go ahead.