[flang] Fix analyze of negate expression

[peixin]

Summary

If the negate expression is not in the specification part of module
or submodule, it should be analyzed. Otherwise, the lowering will
fail when it checks if the negate expression is analyzed. The real
cases are in https://github.com/llvm/llvm-project/issues/55966.

[kiranchandramohan]

flang/lib/Semantics/expression.cpp

2674

Why is this applicable only for Negate?

[peixin]

flang/lib/Semantics/expression.cpp

2674

The patch bringing in this bug is aimed to fix the issue https://github.com/llvm/llvm-project/issues/55086. -2147483647-1 is the special case in module/submodule and supporting it is the portability issue beyond Fortran 2018.

[kiranchandramohan]

flang/lib/Semantics/expression.cpp

2674

Thanks @peixin for the fix. I was just wondering why similar checks are not there in other cases, like Not or Unary Plus.

[peixin]

flang/lib/Semantics/expression.cpp

2674

OK. I will wait for @klausler 's confirmation for this patch.

[peixin]

@jeanPerier Could you please check if your fix (D127735) can handle this case?

[peixin]

flang/lib/Semantics/expression.cpp

2674

I have checked current main, and D127735 can fix this problem, too. However, I think it still needs analyze the negate expression when it is not in module/submodule since doing the similar analysis as the unary plus expression is reasonable. What do you think? @jeanPerier Would it affect the lowering if the negate expression in execution part not analyzed?

[jeanPerier]

However, I think it still needs analyze the negate expression when it is not in module/submodule since doing the similar analysis as the unary plus expression is reasonable. What do you think? @jeanPerier Would it affect the lowering if the negate expression in execution part not analyzed?

I am a bit lost here, is it not analyzed currently ? Analyze seems to be called in the if (const auto *litConst{..... It would be an issue if it is not analyzed in lowering: lowering relies on evaluate::Expr, not the parse tree expression.

Where in you example is the expression not analyzed yet ?

[peixin]

I am a bit lost here, is it not analyzed currently ? Analyze seems to be called in the if (const auto *litConst{.....

No, it does not analyze the negate expression entirely.

Where in you example is the expression not analyzed yet ?

You can check the variable j in the test case of this patch. Without this patch, the result of dumping parse tree is as follows:

| | | | | | Name = 'j'
| | | | | | Initialization -> Constant -> Expr = '-2147483648_4'
| | | | | | | Subtract
| | | | | | | | Expr = '-2147483647_4'
| | | | | | | | | Negate -> Expr -> LiteralConstant -> IntLiteralConstant = '2147483647'
| | | | | | | | Expr = '1_4'
| | | | | | | | | LiteralConstant -> IntLiteralConstant = '1'

If the negate expression is analyzed, it is as follows:

! CHECK: | | Expr = '-2147483647_4'
! CHECK: | | | Negate -> Expr = '2147483647_4'
! CHECK: | | | | LiteralConstant -> IntLiteralConstant = '2147483647'

Currently, I don't see any failure when testing running results. But I am worried if there will any hidden bug.

[jeanPerier]

Thanks for the answer Peixin, now I see what you meant. I however think that may be a "feature", not a bug. With your patch the following code would be rejected:

  integer(4), parameter :: i = -2147483648_4
end

While it is currently accepted with a warning: "portability: negated maximum INTEGER(KIND=4) literal".

I think the issue here is that if the negate operand evaluate::Expr was built/analyzed when it is a constant, it would not be possible to represent the operand "2147483648_4" as an Expr since this would overflow.
This is not an issue in lowering because if the negate operand is a constant, so is the parent expression containing the negate, which is always analyzed, and lowering never needs to visit the operand parse tree expression (*). In general, lowering always use the top level analyzed evaluate::Expr of a parse-tree expressions, so it is not an issue if some sub-expression parser::Expr are not analyzed, although I agree it is not very nice.

(*) The exception being the PFT visit to detect all symbols appearing a program parse-tree, but if a sub-node is not analyzed, the visit simply consider there is no symbols there.

[peixin]

In D127525#3588438, @jeanPerier wrote:

Thanks for the answer Peixin, now I see what you meant. I however think that may be a "feature", not a bug. With your patch the following code would be rejected:

  integer(4), parameter :: i = -2147483648_4
end

While it is currently accepted with a warning: "portability: negated maximum INTEGER(KIND=4) literal".

I think the issue here is that if the negate operand evaluate::Expr was built/analyzed when it is a constant, it would not be possible to represent the operand "2147483648_4" as an Expr since this would overflow.
This is not an issue in lowering because if the negate operand is a constant, so is the parent expression containing the negate, which is always analyzed, and lowering never needs to visit the operand parse tree expression (*). In general, lowering always use the top level analyzed evaluate::Expr of a parse-tree expressions, so it is not an issue if some sub-expression parser::Expr are not analyzed, although I agree it is not very nice.

(*) The exception being the PFT visit to detect all symbols appearing a program parse-tree, but if a sub-node is not analyzed, the visit simply consider there is no symbols there.

Got it. Thanks for the explanations and confirmation that lowering is fine with this.

flang/lib/Semantics/expression.cpp

static MaybeExpr NumericUnaryHelper(ExpressionAnalyzer &context,
   return std::nullopt;
 }
 
 MaybeExpr ExpressionAnalyzer::Analyze(const parser::Expr::UnaryPlus &x) {
   return NumericUnaryHelper(*this, NumericOperator::Add, x);
 }
 
 MaybeExpr ExpressionAnalyzer::Analyze(const parser::Expr::Negate &x) {
+  const auto &scope{
+      this->context().FindScope(this->GetContextualMessages().at())};
+  if (scope.IsModule() || scope.IsSubmodule()) {
     if (const auto *litConst{
             std::get_if<parser::LiteralConstant>(&x.v.value().u)}) {
       if (const auto *intConst{
               std::get_if<parser::IntLiteralConstant>(&litConst->u)}) {
         return Analyze(*intConst, true);
       }
     }
+  }
   return NumericUnaryHelper(*this, NumericOperator::Subtract, x);
 }
 
 MaybeExpr ExpressionAnalyzer::Analyze(const parser::Expr::NOT &x) {
   ArgumentAnalyzer analyzer{*this};
   analyzer.Analyze(x.v);
   if (!analyzer.fatalErrors()) {
     if (analyzer.IsIntrinsicLogical()) {

flang/test/Semantics/negate-analyze.f90

+! RUN: %flang_fc1 -fdebug-dump-parse-tree %s 2>&1 | FileCheck %s
+! Test the analyze of negate expression.
+
+! CHECK: Name = 'i'
+! CHECK: Initialization -> Constant -> Expr = '-2147483648_4'
+! CHECK: | Subtract
+! CHECK: | | Expr = '-2147483647_4'
+! CHECK: | | | Negate -> Expr -> LiteralConstant -> IntLiteralConstant = '2147483647'
+! CHECK: | | Expr = '1_4'
+! CHECK: | | | LiteralConstant -> IntLiteralConstant = '1'
+
+! CHECK: Name = 'j'
+! CHECK: Initialization -> Constant -> Expr = '-2147483648_4'
+! CHECK: | Subtract
+! CHECK: | | Expr = '-2147483647_4'
+! CHECK: | | | Negate -> Expr = '2147483647_4'
+! CHECK: | | | | LiteralConstant -> IntLiteralConstant = '2147483647'
+! CHECK: | | Expr = '1_4'
+! CHECK: | | | LiteralConstant -> IntLiteralConstant = '1'
+
+! CHECK: Name = 'k'
+! CHECK: Initialization -> Constant -> Expr = '-2147483648_4'
+! CHECK: | Subtract
+! CHECK: | | Expr = '-2147483647_4'
+! CHECK: | | | Negate -> Expr -> LiteralConstant -> IntLiteralConstant = '2147483647'
+! CHECK: | | Expr = '1_4'
+! CHECK: | | | LiteralConstant -> IntLiteralConstant = '1'
+
+! CHECK: Variable = 'x'
+! CHECK: | Designator -> DataRef -> Name = 'x'
+! CHECK: Expr = '-1_4'
+! CHECK: | Negate -> Expr = '1_4'
+! CHECK: | | LiteralConstant -> IntLiteralConstant = '1'
+
+! CHECK: Variable = 'x'
+! CHECK: | Designator -> DataRef -> Name = 'x'
+! CHECK: Expr = '-2147483648_4'
+! CHECK: | Subtract
+! CHECK: | | Expr = '-2147483647_4'
+! CHECK: | | | Negate -> Expr = '2147483647_4'
+! CHECK: | | | | LiteralConstant -> IntLiteralConstant = '2147483647'
+! CHECK: | | Expr = '1_4'
+! CHECK: | | | LiteralConstant -> IntLiteralConstant = '1'
+
+module m
+  integer, parameter :: i = -2147483647-1
+contains
+  subroutine sub()
+    integer, parameter :: j = -2147483647-1
+  end
+end
+
+submodule(m) m2
+  integer, parameter :: k = -2147483647-1
+end
+
+program main
+  integer :: x
+  x = -1
+  x = -2147483647-1
+end