[flang-commits] [flang] [flang] Better IS_CONTIGUOUS folding for substrings (PR #115970)

[Peter Klausler via flang-commits]

klausler wrote:

Thanks for looking into that code; I’ve reworked it.

From: jeanPerier ***@***.***>
Date: Wednesday, November 13, 2024 at 02:50
To: llvm/llvm-project ***@***.***>
Cc: Peter Klausler ***@***.***>, Author ***@***.***>
Subject: Re: [llvm/llvm-project] [flang] Better IS_CONTIGUOUS folding for substrings (PR #115970)

@jeanPerier commented on this pull request.

________________________________

In flang/lib/Evaluate/check-expression.cpp<https://github.com/llvm/llvm-project/pull/115970#discussion_r1839947264>:

> +          upperExpr = lenExpr;

+        }

+        if (!upperExpr) {

+          return std::nullopt; // could be empty substrings

+        } else if (auto upper{ToInt64(Fold(context_, std::move(*upperExpr)))}) {

+          if (*upper < *lower) {

+            return true; // empty substrings: vacuously contiguous

+          } else if (*lower > 1) {

+            return false; // lower > 1

+          } else if (lenExpr) {

+            if (auto lenVal{ToInt64(Fold(context_, std::move(*lenExpr)))};

+                lenVal && *lenVal != *upper) {

+              return false; // upper < length

+            }

+          }

+        }

This seems a quite broad fallback, I think the code is incorrectly falling back into it for things like:

subroutine test2(charr, n)

  character(10) :: charr(5)

  integer :: n

  print *, is_contiguous(charr(:)(1:n))

end subroutine

This folds would now fold to true, while this is cannot be folded (and will be false if n is not 10 or smaller than 1 at runtime).

It seems the code can only fall back to "return result" when it can be determined that the lower bound is one, and the upper bound is absent or equal to the length.

________________________________

In flang/lib/Evaluate/check-expression.cpp<https://github.com/llvm/llvm-project/pull/115970#discussion_r1839963549>:

> +            return true; // empty substrings: vacuously contiguous

+          } else if (*lower > 1) {

+            return false; // lower > 1

+          } else if (lenExpr) {

+            if (auto lenVal{ToInt64(Fold(context_, std::move(*lenExpr)))};

+                lenVal && *lenVal != *upper) {

+              return false; // upper < length

+            }

+          }

+        }

+      } else {

+        return std::nullopt; // lower bound not known

+      }

+    }

+    return result;

+  }

For some reason I do not explain to myself it seems your patch is incorrectly causing the folowing is_contiguous to fold to true while the substring is not empty and the base is not contiguous:

subroutine test(charr)

  character(10) :: charr(5)

  print *, is_contiguous(charr(1:5:2)(1:10))

end subroutine

My comment about incorrectly falling back into the "return is_contigous(base)" cannot explain that since the base is not contiguous...

—
Reply to this email directly, view it on GitHub<https://github.com/llvm/llvm-project/pull/115970#pullrequestreview-2432546834>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AIRI5XISOSE6PNHMHPOM2LT2AMVH7AVCNFSM6AAAAABRVIVFA6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMZSGU2DMOBTGQ>.
You are receiving this because you authored the thread.Message ID: ***@***.***>


https://github.com/llvm/llvm-project/pull/115970