Page MenuHomePhabricator

[flang] Change the default F18_FC to gfortran
ClosedPublic

Authored by AlexisPerry on Jul 9 2020, 9:01 AM.

Details

Summary

Changed default F18_FC from pgf90 to gfortran. Removed unnecessary references to pgf90 in favor of more generic naming.

Diff Detail

Event Timeline

AlexisPerry created this revision.Jul 9 2020, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2020, 9:01 AM
AlexisPerry added a project: Restricted Project.Jul 9 2020, 9:05 AM

Extended the flang driver options to include gfortran equivalents to pgf90 specific options.

I don't see the previous change made to set gfortran as default. Are they gone with your diff update?

Changed the default external compiler used by the flang temporary driver.

  • Changed default F18_FC from pgf90 to gfortran.
  • Removed unneccesary references to pgf90 in favor of more generic naming.
  • Extended the flang driver options to include gfortran equivalents to pgf90 specific options.

I don't see the previous change made to set gfortran as default. Are they gone with your diff update?

My mistake, sorry about that. They should be back now.

It's weird, the changes are in but both files are identical ... so it looks like there is nothing to review

I'll admit, this is my first time trying to commit to LLVM directly and using Phabricator (rather than on a fork that uses pull requests), and I'm definitely still learning the workflow. I had some git issues when first creating the patch that meant my first commit was lost locally, so when I went to update the patch it only had the second commit which caused the original changes to disappear. I then created a new branch and re-did the changes and updated again, using only a single commit, so that the diff would now have everything in it.

If this has resulted in an unreviewable state for this diff, then I will close it and submit a new one which will hopefully go more smoothly now that I've learned a few things. Sorry for the confusion.

I'll admit, this is my first time trying to commit to LLVM directly and using Phabricator (rather than on a fork that uses pull requests), and I'm definitely still learning the workflow. I had some git issues when first creating the patch that meant my first commit was lost locally, so when I went to update the patch it only had the second commit which caused the original changes to disappear. I then created a new branch and re-did the changes and updated again, using only a single commit, so that the diff would now have everything in it.

If this has resulted in an unreviewable state for this diff, then I will close it and submit a new one which will hopefully go more smoothly now that I've learned a few things. Sorry for the confusion.

The diff looks fine now! If you don't use it already, I would recommend to use arcanist to deal with the diff and phabricator ... it makes things easier

clementval accepted this revision.Jul 9 2020, 5:23 PM

LGTM. You might want to wait to see if anybody has something to say on this.

This revision is now accepted and ready to land.Jul 9 2020, 5:23 PM

Thanks for picking up the work on this one @AlexisPerry!

I have replied to the RFC (late - my apologies) with a slightly different proposal. I'd like to make a final decision at the developers call on Monday. I don't hold my position too strongly though and would be happy to go in this direction. I also think we can go ahead with this patch because it moves us to a better place than we are now. Even if we chose my proposal to have no default most of this patch would still be kept because having gfortran support is a useful feature anyway.

On the patch itself, I have two requests:

  1. This patch seems to be doing a couple of different things now that don't seem to be strictly linked:
    • Switching the default F18_FC to gfortran
    • Add support for a number of new gfortran options

I think these would be better submitted as two patches. Do you agree?

  1. The patch should add a regression test for these fallback and option translation behaviours in test/Driver. It is a shame there are none already.

+1 to split this in two patches

sscalpone accepted this revision.Jul 10 2020, 8:15 AM

Splitting this into two patches as requested by reviewers.

Change the default external compiler used by the flang temporary driver

- Changed default F18_FC from pgf90 to gfortran.
- Removed unneccesary references to pgf90 in favor of more generic naming.

The second part of the split can be found here: https://reviews.llvm.org/D83687

richard.barton.arm requested changes to this revision.Jul 13 2020, 10:18 AM

Request one change to OpenMP handling, but the rest looks fine.

flang/tools/f18/f18.cpp
644–645

This needs to be amended to support gfortran. We'd need a similar switch on isPGF90 to pass -fopenmp here. I think it might work better to be moved into the next if/else block so it can be set to gfortran/pgfortran there.

Might also be better to add this as part of D83687 rather than this patch.

This revision now requires changes to proceed.Jul 13 2020, 10:18 AM
AlexisPerry marked an inline comment as done.Jul 13 2020, 11:08 AM

@richard.barton.arm I have made the requested adjustments in the other patch. Thanks for the feedback!

flang/tools/f18/f18.cpp
644–645

I agree this would be better as part of D83687, so I will add the fix over there. Good catch!

When I build with this change I'm seeing a couple of modfile tests failing. The problem is that when isPGF90 is false, parser::useHexadecimalEscapeSequences is set to be true which affects how escape sequences are written in character literals in modfiles.

The choice of the compiler shouldn't be affecting modfile format, so I've created https://reviews.llvm.org/D83703 to fix that.

Updates following a rebase on master (due to changes on D83687)

@richard.barton.arm Is this patch acceptable now? Thanks!

LGTM - thanks for doing this one for splitting the patch!

This revision is now accepted and ready to land.Jul 14 2020, 10:36 AM
This revision was automatically updated to reflect the committed changes.