Skip to content

[flang][clang] Add support for -finit-logical in Flang #150939

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[flang][clang] Add support for -finit-logical in Flang #150939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6960,14 +6960,18 @@ def A_DASH : Joined<["-"], "A-">, Group<gfortran_Group>;
def static_libgfortran : Flag<["-"], "static-libgfortran">, Group<gfortran_Group>;

// "f" options with values for gfortran.
// Some of these options are visible for LLVM Flang too.
def fblas_matmul_limit_EQ : Joined<["-"], "fblas-matmul-limit=">, Group<gfortran_Group>;
def fcheck_EQ : Joined<["-"], "fcheck=">, Group<gfortran_Group>;
def fcoarray_EQ : Joined<["-"], "fcoarray=">, Group<gfortran_Group>;
def ffpe_trap_EQ : Joined<["-"], "ffpe-trap=">, Group<gfortran_Group>;
def ffree_line_length_VALUE : Joined<["-"], "ffree-line-length-">, Group<gfortran_Group>;
def finit_character_EQ : Joined<["-"], "finit-character=">, Group<gfortran_Group>;
def finit_integer_EQ : Joined<["-"], "finit-integer=">, Group<gfortran_Group>;
def finit_logical_EQ : Joined<["-"], "finit-logical=">, Group<gfortran_Group>;
def finit_logical_EQ : Joined<["-"], "finit-logical=">,
Group<gfortran_Group>,
Copy link
Contributor

@kiranchandramohan kiranchandramohan Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be moved from the gfortran group.

Copy link
Contributor Author

@NimishMishra NimishMishra Aug 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Kiran, Thanks for your review. I have been reviewing your PR sometime back (which btw adds support for another GNU option in LLVM Flang): #122144. Are you suggesting something similar? Another possibility is that we define a new group containing flags which are shared between LLVM Flang and gfortran. Which one do you think would be preferable?

Visibility<[FlangOption, FC1Option]>,
Copy link
Contributor

@kiranchandramohan kiranchandramohan Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a driver test as well.

HelpText<"Initialize logical type.">;
Copy link
Contributor

@kiranchandramohan kiranchandramohan Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: list the possible values here.

def finit_real_EQ : Joined<["-"], "finit-real=">, Group<gfortran_Group>;
def fmax_array_constructor_EQ : Joined<["-"], "fmax-array-constructor=">, Group<gfortran_Group>;
def fmax_errors_EQ : Joined<["-"], "fmax-errors=">, Group<gfortran_Group>;
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/Driver/ToolChains/Flang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,8 @@ void Flang::addCodegenOptions(const ArgList &Args,
options::OPT_flang_deprecated_no_hlfir,
options::OPT_fno_ppc_native_vec_elem_order,
options::OPT_fppc_native_vec_elem_order, options::OPT_finit_global_zero,
options::OPT_fno_init_global_zero, options::OPT_frepack_arrays,
options::OPT_fno_repack_arrays,
options::OPT_finit_logical_EQ, options::OPT_fno_init_global_zero,
Copy link
Contributor

@tarunprabhu tarunprabhu Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: It may be better to add OPT_finit_logical_EQ before OPT_finit_global_zero. This way, we don't split OPT_finit_global_zero and OPT_fno_init_global_zero since the OPT_<name> and OPT_no_<name> values are typically listed next to one another here.

options::OPT_frepack_arrays, options::OPT_fno_repack_arrays,
options::OPT_frepack_arrays_contiguity_EQ,
options::OPT_fstack_repack_arrays, options::OPT_fno_stack_repack_arrays,
options::OPT_ftime_report, options::OPT_ftime_report_EQ,
Expand Down
6 changes: 6 additions & 0 deletions flang/include/flang/Lower/LoweringOptions.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,5 +74,11 @@ ENUM_LOWERINGOPT(SkipExternalRttiDefinition, unsigned, 1, 0)
/// If false, lower to the complex dialect of MLIR.
/// On by default.
ENUM_LOWERINGOPT(ComplexDivisionToRuntime, unsigned, 1, 1)

/// Initialization for logical type
/// -1 : No initialization
/// 0 : Initialized to .FALSE.
Copy link
Contributor

@tarunprabhu tarunprabhu Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Perhaps align the colons.

/// 1 : Initialized to .TRUE.
ENUM_LOWERINGOPT(LogicalInit, signed, 2, -1)
#undef LOWERINGOPT
#undef ENUM_LOWERINGOPT
16 changes: 16 additions & 0 deletions flang/lib/Frontend/CompilerInvocation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1541,6 +1541,22 @@ bool CompilerInvocation::createFromArgs(
else
invoc.loweringOpts.setInitGlobalZero(false);

// -finit-logical
if (const auto *arg =
args.getLastArg(clang::driver::options::OPT_finit_logical_EQ)) {
llvm::StringRef argValue = llvm::StringRef(arg->getValue());
if (argValue.lower() == "true")
Copy link
Contributor

@tarunprabhu tarunprabhu Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using StringRef::compare_insensitive here. lower() creates a new std::string which seems unnecessary.

invoc.loweringOpts.setLogicalInit(1);
else if (argValue.lower() == "false")
invoc.loweringOpts.setLogicalInit(0);
Comment on lines +1548 to +1551
Copy link
Contributor

@kiranchandramohan kiranchandramohan Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: braces to match the else.

else {
const unsigned diagID = diags.getCustomDiagID(
Copy link
Contributor

@tarunprabhu tarunprabhu Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use an existing clang driver diagnostic here instead of creating custom id? For instance

 diags.Report(clang::diag::err_drv_invalid_value_with_suggestion)
          << arg->getAsString(args) << arg->getValue() << "false true";

Copy link
Contributor Author

@NimishMishra NimishMishra Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I did not know these. Will change.

clang::DiagnosticsEngine::Error,
"Invalid argument to -finit-logical. Must be <true/false>");
diags.Report(diagID);
}
}

// Preserve all the remark options requested, i.e. -Rpass, -Rpass-missed or
// -Rpass-analysis. This will be used later when processing and outputting the
// remarks generated by LLVM in ExecuteCompilerInvocation.cpp.
Expand Down
73 changes: 73 additions & 0 deletions flang/lib/Lower/Bridge.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5720,6 +5720,79 @@ class FirConverter : public Fortran::lower::AbstractConverter {
void instantiateVar(const Fortran::lower::pft::Variable &var,
Fortran::lower::AggregateStoreMap &storeMap) {
Fortran::lower::instantiateVariable(*this, var, localSymbols, storeMap);

/// Implicit assignment is defined by the `-finit-*` family of flags.
/// These options do not initialize:
/// 1) Any variable already initialized
/// 2) objects with the POINTER attribute
/// 3) allocatable arrays
/// 4) variables that appear in an EQUIVALENCE statement
Comment on lines +5726 to +5729
Copy link
Contributor

@kiranchandramohan kiranchandramohan Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this to match the implementation in gfortran?

Copy link
Contributor Author

@NimishMishra NimishMishra Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. These are taken from https://gcc.gnu.org/onlinedocs/gfortran/Code-Gen-Options.html. Would that be okay?


auto isEligibleForImplicitAssignment = [&var]() -> bool {
if (!var.hasSymbol())
return false;

const Fortran::semantics::Symbol &sym = var.getSymbol();
if (const auto *details =
sym.detailsIf<Fortran::semantics::ObjectEntityDetails>()) {
if (details->init())
return false;
}

if (sym.attrs().test(Fortran::semantics::Attr::POINTER))
return false;

if (sym.Rank() > 0 &&
sym.attrs().test(Fortran::semantics::Attr::ALLOCATABLE))
return false;

if (Fortran::lower::pft::getDependentVariableList(sym).size() > 1)
return false;

return true;
};

auto processImplicitAssignment = [&]() -> void {
const Fortran::semantics::Symbol &sym = var.getSymbol();
const Fortran::semantics::DeclTypeSpec *declTy = sym.GetType();
bool isInitLogicalFlagDefined =
(getLoweringOptions().getLogicalInit() == 1 ||
getLoweringOptions().getLogicalInit() == 0);

/*
* Process -finit-logical=true|false
* Create an implicit assignment of form `var = value`,
* where `value` is either true or false, and generically
* build the assignment.
*/
if (isInitLogicalFlagDefined &&
declTy->category() ==
Fortran::semantics::DeclTypeSpec::Category::Logical) {
Fortran::parser::Expr expr =
Fortran::parser::Expr{Fortran::parser::LiteralConstant{
Fortran::parser::LogicalLiteralConstant{
(getLoweringOptions().getLogicalInit() == 0) ? false : true,
std::optional<Fortran::parser::KindParam>{}}}};
Fortran::parser::Designator designator = Fortran::parser::Designator{
Fortran::parser::DataRef{Fortran::parser::Name{
Fortran::parser::FindSourceLocation(sym.name()),
const_cast<Fortran::semantics::Symbol *>(&sym)}}};
designator.source = Fortran::parser::FindSourceLocation(sym.name());
Fortran::parser::Variable variable = Fortran::parser::Variable{
Fortran::common::Indirection<Fortran::parser::Designator>{
std::move(designator)}};
Fortran::parser::AssignmentStmt stmt = Fortran::parser::AssignmentStmt{
std::make_tuple(std::move(variable), std::move(expr))};
Fortran::evaluate::ExpressionAnalyzer ea{bridge.getSemanticsContext()};
const Fortran::evaluate::Assignment *assign = ea.Analyze(stmt);
if (assign)
genAssignment(*assign);
}
Comment on lines +5768 to +5790
Copy link
Contributor

@kiranchandramohan kiranchandramohan Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No action required. But have you considered just generating an hlfir.assign?

Copy link
Contributor Author

@NimishMishra NimishMishra Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not checked. Thanks for the suggestion, let me explore that and get back.

};

if (isEligibleForImplicitAssignment())
processImplicitAssignment();

if (var.hasSymbol())
genOpenMPSymbolProperties(*this, var);
}
Expand Down
82 changes: 82 additions & 0 deletions flang/test/Lower/logical_init.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
! RUN: %flang_fc1 -emit-fir -o - %s | FileCheck --check-prefix=CHECK-UNINT %s
! RUN: %flang_fc1 -emit-fir -finit-logical=true -o - %s | FileCheck --check-prefix=CHECK-TRUE %s
! RUN: %flang_fc1 -emit-fir -finit-logical=false -o - %s | FileCheck --check-prefix=CHECK-FALSE %s

subroutine logical_scalar
!CHECK-UNINT-NOT: {{.}} = fir.convert %true : (i1) -> !fir.logical<4>
!CHECK-UNINT-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>

!CHECK-TRUE: {{.}} = fir.convert %true : (i1) -> !fir.logical<4>
!CHECK-TRUE-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>

!CHECK-FALSE: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>
!CHECK-FALSE-NOT: {{.}} = fir.convert %true : (i1) -> !fir.logical<4>
Comment on lines +6 to +13
Copy link
Contributor

@kiranchandramohan kiranchandramohan Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is probably better to test that the variable is assigned the relevant value.

logical :: x
end subroutine


subroutine logical_allocatable
!CHECK-UNINT-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>
!CHECK-UINIT-NOT: {{.*}} = fir.convert %true : (i1) -> !fir.logical<4>

!CHECK-TRUE: {{.}} = fir.convert %true : (i1) -> !fir.logical<4>
!CHECK-TRUE-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>

!CHECK-FALSE: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>
!CHECK-FALSE-NOT: {{.}} = fir.convert %true : (i1) -> !fir.logical<4>
logical, allocatable :: x
end subroutine


subroutine logical_array
!CHECK-UNINT-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>
!CHECK-UINIT-NOT: {{.*}} = fir.convert %true : (i1) -> !fir.logical<4>

!CHECK-TRUE: {{.}} = fir.convert %true : (i1) -> !fir.logical<4>
!CHECK-TRUE-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>

!CHECK-FALSE: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>
!CHECK-FALSE-NOT: {{.}} = fir.convert %true : (i1) -> !fir.logical<4>
logical :: x(5)
end subroutine


subroutine logical_pointer
!CHECK-UNINT-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>
!CHECK-UINIT-NOT: {{.*}} = fir.convert %true : (i1) -> !fir.logical<4>

!CHECK-TRUE-NOT: {{.}} = fir.convert %true : (i1) -> !fir.logical<4>
!CHECK-TRUE-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>

!CHECK-FALSE-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>
!CHECK-FALSE-NOT: {{.}} = fir.convert %true : (i1) -> !fir.logical<4>
logical, pointer :: x
end subroutine


subroutine logical_allocatable_array
!CHECK-UNINT-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>
!CHECK-UINIT-NOT: {{.*}} = fir.convert %true : (i1) -> !fir.logical<4>

!CHECK-TRUE-NOT: {{.}} = fir.convert %true : (i1) -> !fir.logical<4>
!CHECK-TRUE-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>

!CHECK-FALSE-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>
!CHECK-FALSE-NOT: {{.}} = fir.convert %true : (i1) -> !fir.logical<4>
logical, allocatable :: x(:)
end subroutine


subroutine logical_in_equivalence
!CHECK-UNINT-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>
!CHECK-UINIT-NOT: {{.*}} = fir.convert %true : (i1) -> !fir.logical<4>

!CHECK-TRUE-NOT: {{.}} = fir.convert %true : (i1) -> !fir.logical<4>
!CHECK-TRUE-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>

!CHECK-FALSE-NOT: {{.*}} = fir.convert %false : (i1) -> !fir.logical<4>
!CHECK-FALSE-NOT: {{.}} = fir.convert %true : (i1) -> !fir.logical<4>
logical :: x
real :: y
equivalence(x,y)
end subroutine
Copy link
Contributor

@kiranchandramohan kiranchandramohan Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Add a check that derived components are not affected.