[flang] Treat pre-processed input as fixed #117563
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-flang-parser @llvm/pr-subscribers-clang Author: None (macurtis-amd) ChangesFixes an issue introduced by 9fb2db1 [flang] Retain spaces when preprocessing fixed-form source Where flang -fc1 fails to parse preprocessor output because it now remains in fixed form. Full diff: https://github.com/llvm/llvm-project/pull/117563.diff 6 Files Affected:
diff --git a/clang/include/clang/Driver/Types.def b/clang/include/clang/Driver/Types.def
index 214c5e7a789f97..5b64700c6a688b 100644
--- a/clang/include/clang/Driver/Types.def
+++ b/clang/include/clang/Driver/Types.def
@@ -88,8 +88,11 @@ TYPE("assembler-with-cpp", Asm, PP_Asm, "S", phases
// modules when Flang needs to emit pre-processed files. Therefore, the
// `PP_TYPE` is set to `PP_Fortran` so that the driver is fine with
// "pre-processing a pre-processed file".
-TYPE("f95", PP_Fortran, PP_Fortran, "i", phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
-TYPE("f95-cpp-input", Fortran, PP_Fortran, nullptr, phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("f95", PP_Fortran, PP_Fortran, "i", phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("f95-cpp-input", Fortran, PP_Fortran, nullptr, phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("f95-fixed", PP_Fortran_Fixed, PP_Fortran_Fixed, "i", phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("f95-fixed-cpp-input", Fortran_Fixed, PP_Fortran_Fixed, nullptr, phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+
TYPE("java", Java, INVALID, nullptr, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
// LLVM IR/LTO types. We define separate types for IR and LTO because LTO
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 0d426a467e9a3b..768e2e7842650c 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -1037,10 +1037,14 @@ types::ID ToolChain::LookupTypeForExtension(StringRef Ext) const {
types::ID id = types::lookupTypeForExtension(Ext);
// Flang always runs the preprocessor and has no notion of "preprocessed
- // fortran". Here, TY_PP_Fortran is coerced to TY_Fortran to avoid treating
- // them differently.
- if (D.IsFlangMode() && id == types::TY_PP_Fortran)
- id = types::TY_Fortran;
+ // fortran". Here, TY_PP_Fortran[_Fixed] is coerced to TY_Fortran[_Fixed] to
+ // avoid treating them differently.
+ if (D.IsFlangMode()) {
+ if (id == types::TY_PP_Fortran)
+ id = types::TY_Fortran;
+ else if (id == types::TY_PP_Fortran_Fixed)
+ id = types::TY_Fortran_Fixed;
+ }
return id;
}
diff --git a/clang/lib/Driver/Types.cpp b/clang/lib/Driver/Types.cpp
index eaca74a7b55292..19e2c55d4a663a 100644
--- a/clang/lib/Driver/Types.cpp
+++ b/clang/lib/Driver/Types.cpp
@@ -162,7 +162,9 @@ bool types::isAcceptedByFlang(ID Id) {
return false;
case TY_Fortran:
+ case TY_Fortran_Fixed:
case TY_PP_Fortran:
+ case TY_PP_Fortran_Fixed:
return true;
case TY_LLVM_IR:
case TY_LLVM_BC:
@@ -300,8 +302,8 @@ types::ID types::lookupTypeForExtension(llvm::StringRef Ext) {
return llvm::StringSwitch<types::ID>(Ext)
.Case("c", TY_C)
.Case("C", TY_CXX)
- .Case("F", TY_Fortran)
- .Case("f", TY_PP_Fortran)
+ .Case("F", TY_Fortran_Fixed)
+ .Case("f", TY_PP_Fortran_Fixed)
.Case("h", TY_CHeader)
.Case("H", TY_CXXHeader)
.Case("i", TY_PP_C)
@@ -344,10 +346,10 @@ types::ID types::lookupTypeForExtension(llvm::StringRef Ext) {
.Case("f90", TY_PP_Fortran)
.Case("F95", TY_Fortran)
.Case("f95", TY_PP_Fortran)
- .Case("for", TY_PP_Fortran)
- .Case("FOR", TY_PP_Fortran)
- .Case("fpp", TY_Fortran)
- .Case("FPP", TY_Fortran)
+ .Case("for", TY_PP_Fortran_Fixed)
+ .Case("FOR", TY_PP_Fortran_Fixed)
+ .Case("fpp", TY_Fortran_Fixed)
+ .Case("FPP", TY_Fortran_Fixed)
.Case("gch", TY_PCH)
.Case("hip", TY_HIP)
.Case("hipi", TY_PP_HIP)
diff --git a/flang/lib/Frontend/CompilerInvocation.cpp b/flang/lib/Frontend/CompilerInvocation.cpp
index 0b79c95eade0d3..acd884fb10b3fe 100644
--- a/flang/lib/Frontend/CompilerInvocation.cpp
+++ b/flang/lib/Frontend/CompilerInvocation.cpp
@@ -639,6 +639,7 @@ static bool parseFrontendArgs(FrontendOptions &opts, llvm::opt::ArgList &args,
// Get the input kind (from the value passed via `-x`)
InputKind dashX(Language::Unknown);
+ FortranForm dashXForm = FortranForm::Unknown;
if (const llvm::opt::Arg *a =
args.getLastArg(clang::driver::options::OPT_x)) {
llvm::StringRef xValue = a->getValue();
@@ -648,6 +649,8 @@ static bool parseFrontendArgs(FrontendOptions &opts, llvm::opt::ArgList &args,
// pre-processed inputs.
.Case("f95", Language::Fortran)
.Case("f95-cpp-input", Language::Fortran)
+ .Case("f95-fixed", Language::Fortran)
+ .Case("f95-fixed-cpp-input", Language::Fortran)
// CUDA Fortran
.Case("cuda", Language::Fortran)
.Default(Language::Unknown);
@@ -663,6 +666,13 @@ static bool parseFrontendArgs(FrontendOptions &opts, llvm::opt::ArgList &args,
if (dashX.isUnknown())
diags.Report(clang::diag::err_drv_invalid_value)
<< a->getAsString(args) << a->getValue();
+
+ if (dashX.getLanguage() == Language::Fortran) {
+ if (xValue.starts_with("f95-fixed"))
+ dashXForm = FortranForm::FixedForm;
+ else
+ dashXForm = FortranForm::FreeForm;
+ }
}
// Collect the input files and save them in our instance of FrontendOptions.
@@ -694,6 +704,8 @@ static bool parseFrontendArgs(FrontendOptions &opts, llvm::opt::ArgList &args,
arg->getOption().matches(clang::driver::options::OPT_ffixed_form)
? FortranForm::FixedForm
: FortranForm::FreeForm;
+ } else if (dashXForm != FortranForm::Unknown) {
+ opts.fortranForm = dashXForm;
}
// Set fixedFormColumns based on -ffixed-line-length=<value>
diff --git a/flang/test/Driver/x-lang.f b/flang/test/Driver/x-lang.f
new file mode 100644
index 00000000000000..08282c9479f55a
--- /dev/null
+++ b/flang/test/Driver/x-lang.f
@@ -0,0 +1,8 @@
+!RUN: %flang -save-temps -### %S/Inputs/free-form-test.f90 2>&1 | FileCheck %s --check-prefix=FREE
+!RUN: %flang -save-temps -### %S/Inputs/fixed-form-test.f 2>&1 | FileCheck %s --check-prefix=FIXED
+
+FREE: "-fc1" {{.*}} "-o" "free-form-test.i" {{.*}} "-x" "f95-cpp-input" "{{.*}}/free-form-test.f90"
+FREE-NEXT: "-fc1" {{.*}} "-o" "free-form-test.bc" {{.*}} "-x" "f95" "free-form-test.i"
+
+FIXED: "-fc1" {{.*}} "-o" "fixed-form-test.i" {{.*}} "-x" "f95-fixed-cpp-input" "{{.*}}/fixed-form-test.f"
+FIXED-NEXT: "-fc1" {{.*}} "-o" "fixed-form-test.bc" {{.*}} "-x" "f95-fixed" "fixed-form-test.i"
diff --git a/flang/test/Parser/x-f95-fixed.f b/flang/test/Parser/x-f95-fixed.f
new file mode 100644
index 00000000000000..2012bfc018ff12
--- /dev/null
+++ b/flang/test/Parser/x-f95-fixed.f
@@ -0,0 +1,17 @@
+ subroutine foo(a, b)
+ if ( (a .eq. 0) .and.(b. eq. 1)) then
+
+ print *, "foo"
+ end if
+ end subroutine
+
+! RUN: %flang_fc1 -fsyntax-only "-x" "f95-fixed" %s 2>&1 | FileCheck %s --allow-empty --check-prefix=F95-FIXED
+! F95-FIXED-NOT: Could not parse {{.*}}x-f95-fixed.f
+! F95-FIXED-NOT: error
+! F95-FIXED-NOT: warning
+
+! RUN: not %flang_fc1 -fsyntax-only "-x" "f95" %s 2>&1 | FileCheck %s --check-prefix=F95 --strict-whitespace
+! F95: error: Could not parse {{.*}}x-f95-fixed.f
+! F95: {{.*}}x-f95-fixed.f:2:31: error: expected ')'
+! F95: if ( (a .eq. 0) .and.(b. eq. 1)) then
+! F95: {{^([ ]{32})}}^
|
|
@llvm/pr-subscribers-clang-driver Author: None (macurtis-amd) ChangesFixes an issue introduced by 9fb2db1 [flang] Retain spaces when preprocessing fixed-form source Where flang -fc1 fails to parse preprocessor output because it now remains in fixed form. Full diff: https://github.com/llvm/llvm-project/pull/117563.diff 6 Files Affected:
diff --git a/clang/include/clang/Driver/Types.def b/clang/include/clang/Driver/Types.def
index 214c5e7a789f97..5b64700c6a688b 100644
--- a/clang/include/clang/Driver/Types.def
+++ b/clang/include/clang/Driver/Types.def
@@ -88,8 +88,11 @@ TYPE("assembler-with-cpp", Asm, PP_Asm, "S", phases
// modules when Flang needs to emit pre-processed files. Therefore, the
// `PP_TYPE` is set to `PP_Fortran` so that the driver is fine with
// "pre-processing a pre-processed file".
-TYPE("f95", PP_Fortran, PP_Fortran, "i", phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
-TYPE("f95-cpp-input", Fortran, PP_Fortran, nullptr, phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("f95", PP_Fortran, PP_Fortran, "i", phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("f95-cpp-input", Fortran, PP_Fortran, nullptr, phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("f95-fixed", PP_Fortran_Fixed, PP_Fortran_Fixed, "i", phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("f95-fixed-cpp-input", Fortran_Fixed, PP_Fortran_Fixed, nullptr, phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+
TYPE("java", Java, INVALID, nullptr, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
// LLVM IR/LTO types. We define separate types for IR and LTO because LTO
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 0d426a467e9a3b..768e2e7842650c 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -1037,10 +1037,14 @@ types::ID ToolChain::LookupTypeForExtension(StringRef Ext) const {
types::ID id = types::lookupTypeForExtension(Ext);
// Flang always runs the preprocessor and has no notion of "preprocessed
- // fortran". Here, TY_PP_Fortran is coerced to TY_Fortran to avoid treating
- // them differently.
- if (D.IsFlangMode() && id == types::TY_PP_Fortran)
- id = types::TY_Fortran;
+ // fortran". Here, TY_PP_Fortran[_Fixed] is coerced to TY_Fortran[_Fixed] to
+ // avoid treating them differently.
+ if (D.IsFlangMode()) {
+ if (id == types::TY_PP_Fortran)
+ id = types::TY_Fortran;
+ else if (id == types::TY_PP_Fortran_Fixed)
+ id = types::TY_Fortran_Fixed;
+ }
return id;
}
diff --git a/clang/lib/Driver/Types.cpp b/clang/lib/Driver/Types.cpp
index eaca74a7b55292..19e2c55d4a663a 100644
--- a/clang/lib/Driver/Types.cpp
+++ b/clang/lib/Driver/Types.cpp
@@ -162,7 +162,9 @@ bool types::isAcceptedByFlang(ID Id) {
return false;
case TY_Fortran:
+ case TY_Fortran_Fixed:
case TY_PP_Fortran:
+ case TY_PP_Fortran_Fixed:
return true;
case TY_LLVM_IR:
case TY_LLVM_BC:
@@ -300,8 +302,8 @@ types::ID types::lookupTypeForExtension(llvm::StringRef Ext) {
return llvm::StringSwitch<types::ID>(Ext)
.Case("c", TY_C)
.Case("C", TY_CXX)
- .Case("F", TY_Fortran)
- .Case("f", TY_PP_Fortran)
+ .Case("F", TY_Fortran_Fixed)
+ .Case("f", TY_PP_Fortran_Fixed)
.Case("h", TY_CHeader)
.Case("H", TY_CXXHeader)
.Case("i", TY_PP_C)
@@ -344,10 +346,10 @@ types::ID types::lookupTypeForExtension(llvm::StringRef Ext) {
.Case("f90", TY_PP_Fortran)
.Case("F95", TY_Fortran)
.Case("f95", TY_PP_Fortran)
- .Case("for", TY_PP_Fortran)
- .Case("FOR", TY_PP_Fortran)
- .Case("fpp", TY_Fortran)
- .Case("FPP", TY_Fortran)
+ .Case("for", TY_PP_Fortran_Fixed)
+ .Case("FOR", TY_PP_Fortran_Fixed)
+ .Case("fpp", TY_Fortran_Fixed)
+ .Case("FPP", TY_Fortran_Fixed)
.Case("gch", TY_PCH)
.Case("hip", TY_HIP)
.Case("hipi", TY_PP_HIP)
diff --git a/flang/lib/Frontend/CompilerInvocation.cpp b/flang/lib/Frontend/CompilerInvocation.cpp
index 0b79c95eade0d3..acd884fb10b3fe 100644
--- a/flang/lib/Frontend/CompilerInvocation.cpp
+++ b/flang/lib/Frontend/CompilerInvocation.cpp
@@ -639,6 +639,7 @@ static bool parseFrontendArgs(FrontendOptions &opts, llvm::opt::ArgList &args,
// Get the input kind (from the value passed via `-x`)
InputKind dashX(Language::Unknown);
+ FortranForm dashXForm = FortranForm::Unknown;
if (const llvm::opt::Arg *a =
args.getLastArg(clang::driver::options::OPT_x)) {
llvm::StringRef xValue = a->getValue();
@@ -648,6 +649,8 @@ static bool parseFrontendArgs(FrontendOptions &opts, llvm::opt::ArgList &args,
// pre-processed inputs.
.Case("f95", Language::Fortran)
.Case("f95-cpp-input", Language::Fortran)
+ .Case("f95-fixed", Language::Fortran)
+ .Case("f95-fixed-cpp-input", Language::Fortran)
// CUDA Fortran
.Case("cuda", Language::Fortran)
.Default(Language::Unknown);
@@ -663,6 +666,13 @@ static bool parseFrontendArgs(FrontendOptions &opts, llvm::opt::ArgList &args,
if (dashX.isUnknown())
diags.Report(clang::diag::err_drv_invalid_value)
<< a->getAsString(args) << a->getValue();
+
+ if (dashX.getLanguage() == Language::Fortran) {
+ if (xValue.starts_with("f95-fixed"))
+ dashXForm = FortranForm::FixedForm;
+ else
+ dashXForm = FortranForm::FreeForm;
+ }
}
// Collect the input files and save them in our instance of FrontendOptions.
@@ -694,6 +704,8 @@ static bool parseFrontendArgs(FrontendOptions &opts, llvm::opt::ArgList &args,
arg->getOption().matches(clang::driver::options::OPT_ffixed_form)
? FortranForm::FixedForm
: FortranForm::FreeForm;
+ } else if (dashXForm != FortranForm::Unknown) {
+ opts.fortranForm = dashXForm;
}
// Set fixedFormColumns based on -ffixed-line-length=<value>
diff --git a/flang/test/Driver/x-lang.f b/flang/test/Driver/x-lang.f
new file mode 100644
index 00000000000000..08282c9479f55a
--- /dev/null
+++ b/flang/test/Driver/x-lang.f
@@ -0,0 +1,8 @@
+!RUN: %flang -save-temps -### %S/Inputs/free-form-test.f90 2>&1 | FileCheck %s --check-prefix=FREE
+!RUN: %flang -save-temps -### %S/Inputs/fixed-form-test.f 2>&1 | FileCheck %s --check-prefix=FIXED
+
+FREE: "-fc1" {{.*}} "-o" "free-form-test.i" {{.*}} "-x" "f95-cpp-input" "{{.*}}/free-form-test.f90"
+FREE-NEXT: "-fc1" {{.*}} "-o" "free-form-test.bc" {{.*}} "-x" "f95" "free-form-test.i"
+
+FIXED: "-fc1" {{.*}} "-o" "fixed-form-test.i" {{.*}} "-x" "f95-fixed-cpp-input" "{{.*}}/fixed-form-test.f"
+FIXED-NEXT: "-fc1" {{.*}} "-o" "fixed-form-test.bc" {{.*}} "-x" "f95-fixed" "fixed-form-test.i"
diff --git a/flang/test/Parser/x-f95-fixed.f b/flang/test/Parser/x-f95-fixed.f
new file mode 100644
index 00000000000000..2012bfc018ff12
--- /dev/null
+++ b/flang/test/Parser/x-f95-fixed.f
@@ -0,0 +1,17 @@
+ subroutine foo(a, b)
+ if ( (a .eq. 0) .and.(b. eq. 1)) then
+
+ print *, "foo"
+ end if
+ end subroutine
+
+! RUN: %flang_fc1 -fsyntax-only "-x" "f95-fixed" %s 2>&1 | FileCheck %s --allow-empty --check-prefix=F95-FIXED
+! F95-FIXED-NOT: Could not parse {{.*}}x-f95-fixed.f
+! F95-FIXED-NOT: error
+! F95-FIXED-NOT: warning
+
+! RUN: not %flang_fc1 -fsyntax-only "-x" "f95" %s 2>&1 | FileCheck %s --check-prefix=F95 --strict-whitespace
+! F95: error: Could not parse {{.*}}x-f95-fixed.f
+! F95: {{.*}}x-f95-fixed.f:2:31: error: expected ')'
+! F95: if ( (a .eq. 0) .and.(b. eq. 1)) then
+! F95: {{^([ ]{32})}}^
|
macurtis-amd
requested review from
banach-space,
klausler and
kiranchandramohan
|
Adding new types to Types.def is a fairly huge design update. I'm not saying this is approach is incorrect, but it definitely deserves a bit of discussion. Perhaps under a GitHub issue? I don't really mind. Could you quickly summarise what's not working and why adding new types to Types.def solves that? Is that the only solution? |
|
Thanks for the fix, I'd like to understand what the bug is better though; what is the difference between the proposed |
|
Thanks @banach-space and @DavidTruby for looking at this.
Created issue 117712. We can continue discussion there if that is the right place.
From that issue: Here is the problematic fixed form fortran source Prior to 9fb2db1 which compiles without error. With 9fb2db1, Which produces: In either case the commands produced by the driver look like:
With this change, the driver produces: Which preserves the fact that reduced.i contains fixed form fortran.
No. I initially looked at modifying Flang::ConstructJob to add It worked, but seemed brittle. It does have the benefit of being more surgical. Another solution I considered, was propagating the fixed vs free information in some way other than the type. I didn't go down this path because it seemed more natural to consider it part of the type. FWIW, I don't have strong feelings here. I'm happy to implement the fix however you all think is best.
Sorry. I should have put this in the commit description, but the issue is specific to the use of |
It seems like something in 9fb2db1 has caused a space to appear between |
|
Ah I see, I can reproduce your issue in the presence of However, Is it something we are expecting to work with the flang compiler driver? I suspected this might be a Windows issue but I can reproduce this on godbolt.org. If -save-temps doesn't work anyway even in simple cases, should we just disable it? Anyways, I think to fix the specific issue you're reporting here, an easier method is rely on the fact that the "preprocessed" output from flang will always be fixed form, regardless of what form the input was in. So instead of modifying It feels wrong that |
You need to add
That's correct. However, LLVM Flang generally follows Clang in the absence of explicit direction from GFortran. This ensures consistent behavior between
Agreed,
@macurtis-amd, thanks for the detailed explanation. The approach you're suggesting seems the most natural, provided we can guarantee or agree on the following:
Alternatively, we could simplify by requiring that |
|
Thanks for the info on -save-temps, I understand it a little better now !
flang -E is already consistent here, it always produces fixed form output regardless of input form. So I think we can just teach flang -fc1 that '.i' files are fixed form. Or just pick another file extension to use ('.fpp'?) and teach it that that is fixed form? I suppose using '.i' doesn't really matter since these files aren't passed to a driver that needs to determine if the file is C or Fortran. If the frontend driver is told that .i files are fixed form then both the examples above and free form examples work for me with -save-temps -fno-integrated-as |
It would be good to document and to test that - perhaps that's already the case?
Agreed, and this should be trivial to implement. Thanks @DavidTruby ! |
|
I'm not sure if it's explicitly documented anywhere but there's a lot of tests, as you can see in the patch that introduced the behaviour here: https://reviews.llvm.org/D106727 Essentially since that patch the output is always valid 72 width fixed form, and if the input was free form it'll also be valid free form (but still valid fixed form too! It's possible to write in a style that can be compiled as both) |
macurtis-amd
force-pushed
the
flang-x-lang
branch
from
9968ef5 to
2b61143
Compare
|
(Sorry for the delayed response ... was on Thanksgiving holiday) @banach-space @DavidTruby Thanks for looking at this! Latest version of the fix simply adds |
macurtis-amd
changed the title
| !Args.hasArg(options::OPT_ffixed_form) && | ||
| !Args.hasArg(options::OPT_ffree_form)) { |
There was a problem hiding this comment.
This should capture the right-most instance of -ffixed-form/-ffree-form, which is exactly what you want here:
Args.getLastArg(clang::driver::options::OPT_ffixed_form,
clang::driver::options::OPT_ffree_form))There was a problem hiding this comment.
Updated to use getLastArg
There was a problem hiding this comment.
I'm not seeing getLastArg here on the github review.. Is this github being weird?
There was a problem hiding this comment.
I'm not seeing
getLastArghere on the github review.. Is this github being weird?
Oops. Really updated to use getLastArg now.
macurtis-amd
force-pushed
the
flang-x-lang
branch
from
2b61143 to
4cbb372
Compare
macurtis-amd
force-pushed
the
flang-x-lang
branch
from
4cbb372 to
d8aed2a
Compare
Happy to help! |
This reverts commit 81d82ca.
This reverts commit 81d82ca.
Fixes an issue introduced by
9fb2db1 [flang] Retain spaces when preprocessing fixed-form source
Where flang -fc1 fails to parse preprocessor output because it now remains in fixed form.