This is an archive of the discontinued LLVM Phabricator instance.

[flang] Run algebraic simplification optimization pass.
ClosedPublic

Authored by vzakhari on Jul 18 2022, 12:17 PM.

Details

Summary

[flang] Run algebraic simplification optimization pass.

Flang algebraic simplification pass will run algebraic simplification
rewrite patterns for Math/Complex/etc. dialects. It is enabled
under opt-for-speed optimization levels (i.e. for O1/O2/O3; Os/Oz will not
enable it).

With this change the FIR/MLIR optimization pipeline becomes affected
by the -O* optimization level switches. Until now these switches
only affected the middle-end and back-end.

Diff Detail

Event Timeline

vzakhari created this revision.Jul 18 2022, 12:17 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 18 2022, 12:17 PM
vzakhari requested review of this revision.Jul 18 2022, 12:17 PM
tschuett added inline comments.
flang/lib/Optimizer/Transforms/AlgebraicSimplification.cpp
20

I assume that this is Flang coding style. Otherwise, you may have a look if you want:
https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

This revision is now accepted and ready to land.Jul 18 2022, 12:26 PM
vzakhari added inline comments.Jul 18 2022, 12:50 PM
flang/lib/Optimizer/Transforms/AlgebraicSimplification.cpp
20

Thanks for catching! Will fix soon.

LGTM

Thank you, @clementval! I will wait to see if @awarzynski has any comments.

vzakhari updated this revision to Diff 445607.Jul 18 2022, 12:57 PM
vzakhari updated this revision to Diff 445666.Jul 18 2022, 6:01 PM

clang-format

Makes sense, thanks for working on this @vzakhari !

This patch introduces the concept of optimisation levels in FIR/MLIR (until now -O{n} would only tweak the middle-end and back-end pipelines). This is a rather large design update - could you mention it in the summary? Also, I would be tempted to move your fix for parseCodeGenArgs to a dedicated patch. More comments inline/below.

From the summary:

[flang] Run algebraic simplification optimization pass.

This patch will only add a placeholder for now, right? Could you point that out?

It is enabled under opt-for-speed optimization levels.

Could you clarify this? In particular, this pass won't be enabled when optimizing for speed and size, right? It's worth mentioning that too.

flang/include/flang/Tools/CLOptions.inc
55

Shouldn't this be consistent with flang-new and set to -O0 instead?

199

UPDATEME ;-) (the signature of the method has changed)

flang/lib/Frontend/CompilerInvocation.cpp
106–112

This is a bug, which I introduced :( Thanks for catching and fixing it! I would create a separate patch with this fix - this is an unrelated change, isn't it? I.e., it applies to -O2 and other optimization flags in general rather than to "AlgebraicSimplification".

Also, I think that this would be a bit more straightforward (at the top of this method):

if (llvm::opt::Arg *a = Args.getLastArg(options::OPT_O_Group)) {
  if (a->getOption().matches(options::OPT_O0))
    return llvm::CodeGenOpt::None;

This way, you will always grab the left-most optimisation flag. Similar code in Clang.

flang/lib/Optimizer/Transforms/AlgebraicSimplification.cpp
10–12
flang/test/Driver/mlir-pass-pipeline.f90
1

You may want to update basic-program.fir as well.

21

Why not keep ALL-LABEL here? Also, an empty line improves readability. At least for me :)

[flang] Run algebraic simplification optimization pass.

This patch will only add a placeholder for now, right? Could you point that out?

It's actually running some simplification if enabled.

[flang] Run algebraic simplification optimization pass.

This patch will only add a placeholder for now, right? Could you point that out?

It's actually running some simplification if enabled.

@clementval is right, the pass does apply Math dialect algebraic simplification patterns.

Makes sense, thanks for working on this @vzakhari !

This patch introduces the concept of optimisation levels in FIR/MLIR (until now -O{n} would only tweak the middle-end and back-end pipelines). This is a rather large design update - could you mention it in the summary? Also, I would be tempted to move your fix for parseCodeGenArgs to a dedicated patch. More comments inline/below.

Thanks for the review! I will update the summary.

From the summary:

[flang] Run algebraic simplification optimization pass.

This patch will only add a placeholder for now, right? Could you point that out?

Replied above.

It is enabled under opt-for-speed optimization levels.

Could you clarify this? In particular, this pass won't be enabled when optimizing for speed and size, right? It's worth mentioning that too.

Correct, it won't be enabled for -Os and -Oz.

flang/include/flang/Tools/CLOptions.inc
55

Makes sense.

199

Thanks! Will fix.

flang/lib/Frontend/CompilerInvocation.cpp
106–112

OK, I will extract it into a separate patch, but can you please explain what change you suggest? Note that I cannot return early due to opts.DebugPassManager setting below.

flang/test/Driver/mlir-pass-pipeline.f90
21

I want to use -NEXT to verify the order of passes precisely. If I use ALL-LABEL, then some passes can sneak in between line 20 and line 21.

vzakhari updated this revision to Diff 445866.Jul 19 2022, 10:24 AM
vzakhari edited the summary of this revision. (Show Details)
awarzynski added inline comments.Jul 19 2022, 11:04 AM
flang/lib/Frontend/CompilerInvocation.cpp
106–112

Not a fan of creating extra work for people. How about this: https://reviews.llvm.org/D130104? Hope you don't mind me taking the liberty to extract that.

flang/test/Driver/mlir-pass-pipeline.f90
21

Ack

Ordering of statistics by name is broken here: https://github.com/llvm/llvm-project/blob/9451440f820a497bd3f9ad0b3a85a7edc7ec67aa/mlir/lib/Pass/PassStatistics.cpp#L38

I believe std::less used by llvm::array_pod_sort_comparator does not make any good for const char *. I will have to fix it, before merging this, otherwise, the LIT tests will sporadicaly fail due to CSE statistics printed in an arbitrary order.

vzakhari updated this revision to Diff 445933.Jul 19 2022, 1:43 PM

Fixed order of statistics checks in the tests.

flang/lib/Frontend/CompilerInvocation.cpp
106–112

Thanks!

This should be good to go now. Please let me know if bbc/tco changes look good to you. In particular, I used O3 as default for them, since they do not have any opt-level options, and I think the expecation is that they will run as many passes as available by default.

Thanks for addressing my comments!

This should be good to go now. Please let me know if bbc/tco changes look good to you. In particular, I used O3 as default for them, since they do not have any opt-level options, and I think the expecation is that they will run as many passes as available by default.

We pledged to retire bbc/tco and that's what I would like for us to aim for long term. Defaulting to -O3 is a bit counter-intuitive - as in, why is this hard-coded? But I don't want to suggest adding support for -O{n} - that would be counter-productive, IMHO. We need -O2, so perhaps that's what we should use here? And if at some point we discover that -O3 is needed instead, then bbc/tco can be updated accordingly.

Thanks for addressing my comments!

This should be good to go now. Please let me know if bbc/tco changes look good to you. In particular, I used O3 as default for them, since they do not have any opt-level options, and I think the expecation is that they will run as many passes as available by default.

We pledged to retire bbc/tco and that's what I would like for us to aim for long term. Defaulting to -O3 is a bit counter-intuitive - as in, why is this hard-coded? But I don't want to suggest adding support for -O{n} - that would be counter-productive, IMHO. We need -O2, so perhaps that's what we should use here? And if at some point we discover that -O3 is needed instead, then bbc/tco can be updated accordingly.

Okay, some compilers use -O2 as a default, so I think it is a little bit better than -O3, though, right now it does not make a difference. I will change the default.

vzakhari updated this revision to Diff 446296.Jul 20 2022, 4:31 PM

Uploading final changes before the merge.

This revision was landed with ongoing or failed builds.Jul 20 2022, 4:43 PM
This revision was automatically updated to reflect the committed changes.
vzakhari updated this revision to Diff 446312.Jul 20 2022, 5:46 PM

I am reopening this for review because of additional changes in CMake files (to fix buildbots failures). Usage of llvm::OptimizationLevel::O2 requires linking with LLVMPasses. Let me know if you have concerns about this.

vzakhari reopened this revision.Jul 20 2022, 5:46 PM
This revision is now accepted and ready to land.Jul 20 2022, 5:46 PM

Uploading final changes before the merge.

Next time, could you wait for all active reviewers to accept first?

Please address the feedback and re-post an updated version of your patch. This cycle continues until all requests and comments have been addressed and a reviewer accepts the patch with a Looks good to me or LGTM.

From how-to-submit-a-patch. LGTM :)

Uploading final changes before the merge.

Next time, could you wait for all active reviewers to accept first?

Yes, I can :) I just thought replacing O3 with O2 was your only concern. The changes seemed straightforward to me, so I went ahead with the merge.

Please address the feedback and re-post an updated version of your patch. This cycle continues until all requests and comments have been addressed and a reviewer accepts the patch with a Looks good to me or LGTM.

From how-to-submit-a-patch. LGTM :)

Thanks!

This revision was automatically updated to reflect the committed changes.

Next time, could you wait for all active reviewers to accept first?

Yes, I can :) I just thought replacing O3 with O2 was your only concern. The changes seemed straightforward to me, so I went ahead with the merge.

That's fair enough. I only asked because my first reaction was that "I haven't approved it yet and this is already merged" :) My bad for not being explicit when I left that comment!