This is an archive of the discontinued LLVM Phabricator instance.

[Flang] Add user option -funderscoring/-fnounderscoring to control trailing underscore added to external names
ClosedPublic

Authored by madanial on Dec 30 2022, 9:13 PM.

Details

Summary

This patch adds user option -funderscoring/-fnounderscoring to control the trailing underscore being appended to external names (e.g. procedure names, common block names). The option in gfortran is documented in https://gcc.gnu.org/onlinedocs/gfortran/Code-Gen-Options.html.

Diff Detail

Event Timeline

madanial created this revision.Dec 30 2022, 9:13 PM
Herald added a project: Restricted Project. · View Herald Transcript
madanial requested review of this revision.Dec 30 2022, 9:13 PM
madanial set the repository for this revision to rG LLVM Github Monorepo.Dec 30 2022, 9:37 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 30 2022, 9:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
clementval requested changes to this revision.Dec 31 2022, 12:35 AM

You need to update the driver-help tests.

flang/include/flang/Tools/CLOptions.inc
194

Please follow the same formatting in this file.

213

Doesn't follow the formatting here.

This revision now requires changes to proceed.Dec 31 2022, 12:35 AM

Hi @madanial , thanks for posting this!

This patch adds user option -funderscoring/-fnounderscoring which behaves similar to the gfortran option be enabling/disabling the ExternalNameConversionPass

I don't quite understand what this option is for and it's hard to deduce from the patch. Please, could you add a link to some documentation? And tests.

-Andrzej

kkwli0 added a comment.Jan 6 2023, 6:25 AM

Hi @madanial , thanks for posting this!

This patch adds user option -funderscoring/-fnounderscoring which behaves similar to the gfortran option be enabling/disabling the ExternalNameConversionPass

I don't quite understand what this option is for and it's hard to deduce from the patch. Please, could you add a link to some documentation? And tests.

-Andrzej

@awarzynski Thanks for the review. @madanial will not be available for the next few weeks. I will try to address some review comments while he is away.

The purpose of this option is to control the trailing underscore being appended to external names (e.g. procedure names, common block names). The option in gfortran is documented in https://gcc.gnu.org/onlinedocs/gfortran/Code-Gen-Options.html.

However, I don't think the patch does what we want. Given a procedure name foo, the -fno-underscoring option will give _QPfoo instead of foo. We will look into it.

However, I don't think the patch does what we want. Given a procedure name foo, the -fno-underscoring option will give _QPfoo instead of foo.

The purpose of this option is to control the trailing underscore being appended to external names (e.g. procedure names, common block names). The option in gfortran is documented in https://gcc.gnu.org/onlinedocs/gfortran/Code-Gen-Options.html.

Thanks for this explanation - much appreciated! Could this short description be added to the summary? Also, could you add a note stating whether the semantics of this option in Flang are consistent with GFortran. Ta!

However, I don't think the patch does what we want. Given a procedure name foo, the -fno-underscoring option will give _QPfoo instead of foo. We will look into it.

Names in Flang are mangled at the FIR level. I couldn't find any documentation for this, but the ExternalNameConversion pass could be helpful.

However, I don't think the patch does what we want. Given a procedure name foo, the -fno-underscoring option will give _QPfoo instead of foo. We will look into it.

Names in Flang are mangled at the FIR level. I couldn't find any documentation for this, but the ExternalNameConversion pass could be helpful.

@awarzynski Thanks for the info. We intend to make use of ExternalNameConversion for this purpose. However, we are not sure how a compiler option can control the behavior in ExternalNameConversion. Any pointer to how other passes do it is appreciated.

madanial updated this revision to Diff 492312.Jan 25 2023, 7:24 PM
madanial retitled this revision from [Flang] Add user option -funderscoring/-fnounderscoring to enable/disable ExternalNameConversionPass to [Flang] Add user option -funderscoring/-fnounderscoring to control trailing underscore added to external names.
madanial edited the summary of this revision. (Show Details)

Implementing functionality to control just the trailing underscore for external names to achieve similar functionality to the option in gfortran, as well as rebase and address review comments.

Small suggestion

flang/lib/Optimizer/Transforms/ExternalNameConversion.cpp
41–45 ↗(On Diff #492312)

To avoid new copy

madanial updated this revision to Diff 492463.Jan 26 2023, 8:33 AM

Addressing review comment

madanial marked an inline comment as done.Jan 26 2023, 8:33 AM

Can you also add test in flang/test/Fir/external-mangling.fir?

Driver changes LGTM, thanks! I will defer to others for changes in other areas.

madanial updated this revision to Diff 496027.Feb 8 2023, 10:44 PM

Rebase, as well as bug fix relating to the ExternalNameConversion pass option that was exposed by the requested fir-opt test case. The pass option was being overwritten by default value whenever it was called through fir-opt.

clementval added inline comments.Feb 9 2023, 1:10 AM
flang/test/Fir/external-mangling.fir
36 ↗(On Diff #496027)

You should check at least the character after it because here the check line would also succeed with an underscore.

39–40 ↗(On Diff #496027)

Same here.

85–88 ↗(On Diff #496027)

Same here.

madanial updated this revision to Diff 497024.Feb 13 2023, 9:42 AM

Addressing the review comments relating to the test case. Thanks for the catch!

madanial marked 3 inline comments as done.Feb 13 2023, 9:43 AM

You still have clang-format issues in your patch. Can you update that.

madanial updated this revision to Diff 497392.EditedFeb 14 2023, 11:06 AM

clang-format issue in ExternalNameConversion.cpp

The pre-commit check is still failing with these tests:

Flang :: Fir/alloc.fir

Flang :: Fir/embox.fir
Flang :: Fir/optional.fir
Flang :: Fir/rebox.fir
Flang :: Lower/common-block.f90
Flang :: Lower/forall/character-1.f90
madanial updated this revision to Diff 497928.Feb 16 2023, 1:32 AM

Bug Fix for failing test cases

You still have style mismatch in flang/include/flang/Tools/CLOptions.inc. This file is likely not run through clang-format so that's why.

flang/include/flang/Tools/CLOptions.inc
196
211–213
218
madanial updated this revision to Diff 497931.EditedFeb 16 2023, 1:47 AM

addressing style mismatch in flang/include/flang/Tools/CLOptions.inc

clementval accepted this revision.Feb 16 2023, 5:05 AM

LGTM. Thanks for working on this.

This revision is now accepted and ready to land.Feb 16 2023, 5:05 AM

Thanks for the review.