Page MenuHomePhabricator

[flang] Upstream two FIR transformation passes
AbandonedPublic

Authored by schweitz on Fri, Jun 26, 4:47 PM.

Details

Summary

Upstream the inliner and do_loop rewriter passes. This diff starts the process of upstreaming some of the FIR transformation passes to llvm-project.

Diff Detail

Event Timeline

schweitz created this revision.Fri, Jun 26, 4:47 PM
Herald added a project: Restricted Project. · View Herald Transcript
sscalpone accepted this revision.Sun, Jun 28, 10:03 AM
sscalpone added inline comments.
flang/include/flang/Optimizer/Transforms/Passes.td
22

Is the a pro forma description that could be added?

This revision is now accepted and ready to land.Sun, Jun 28, 10:03 AM
kiranchandramohan added a project: Restricted Project.Sun, Jun 28, 1:27 PM

Should we add a test? Is tco the tool to use?

richard.barton.arm requested changes to this revision.Mon, Jun 29, 6:07 AM

Stupid question perhaps, but if they are two passes, shouldn't they be added in two commits?

This revision now requires changes to proceed.Mon, Jun 29, 6:07 AM
schweitz abandoned this revision.Mon, Jun 29, 7:32 AM
jdoerfert added a comment.EditedMon, Jun 29, 8:56 AM

Stupid question perhaps, but if they are two passes, shouldn't they be added in two commits?

In the passes files there are 6 pass creators declared. I failed to find all the definitions in this patch.
It seems sensible to split this into multiple commits. I would also recommend tests for new functionality.

[EDIT: Took too long to write my comments, seems abandoned now]

flang/include/flang/Optimizer/Transforms/Passes.td
22

Yes, please.

flang/lib/Optimizer/Transforms/Inliner.cpp
35

I would suggest to make the inliner pass a no-op if it is disabled.
The CFG conversion pass works that way already (I think).

I can also see that the enable flag will prevent it from being put in the pipeline but disabling the create function here with a comment suggesting there is a hidden dependence to another function seems fragile. It is also counter-intuitive to people that have seen llvm::createXXXPass functions before.

flang/lib/Optimizer/Transforms/RewriteLoop.cpp
84

There seems little reason to commit dead code.

schweitz marked an inline comment as done.Mon, Jun 29, 10:24 AM

In the passes files there are 6 pass creators declared. I failed to find all the definitions in this patch.
It seems sensible to split this into multiple commits. I would also recommend tests for new functionality.

[EDIT: Took too long to write my comments, seems abandoned now]

This is the nature of this process we're on. We're being asked to upstream tiny bits with each diff, the smaller the better. The reality is that however one slices it, the code will always be interrelated. In order to keep the diffs small, some sort of compromise must be made.

flang/lib/Optimizer/Transforms/RewriteLoop.cpp
84

I agree that this can be cleaned up.

I disagree in that we will be upstreaming code with TODOs in the short term. The only other approach is to wait until flang is a final polished product, which would make it awkward for the LLVM community as a whole to contribute.

This is the nature of this process we're on. We're being asked to upstream tiny bits with each diff, the smaller the better. The reality is that however one slices it, the code will always be interrelated. In order to keep the diffs small, some sort of compromise must be made

How about series/chain of patches filed together with dependencies[Parent-Child] specified ? This could help in review/tracking process.