[flang] Create temporaries for array sections passed to IGNORE_TKR dummy args #147419

eugeneepshteyn · 2025-07-07T23:03:54Z

Enable creation of a temporary, when array section is passed to IGNORE_TKR dummy argument.

CallInterface::mustBeMadeContiguous() now returns "true" for IGNORE_TKR dummy args, when actual args are passed as array section. This triggers contiguity check already done in the caller and results in creation of a temporary.

If IGNORE_TKR includes c, then don't do contiguity check.

Step towards #138471

If argument is marked as IGNORE_TKR, enable contiguity check on it, unless IGNORE_TKR explicitly specifies to ignore contiguity.

… VOLATILE dummy arg Add the check, which partially addresses llvm#137369 Implement HasTriplet().

… code around

eugeneepshteyn · 2025-07-07T23:04:57Z

flang/lib/Lower/CallInterface.cpp

+  if (dummy->ignoreTKR.test(common::IgnoreTKR::Contiguous))
+    return false;
+
+  // TODO: should this check ignore "device" or "managed"?


Reviewers, please answer this question

eugeneepshteyn · 2025-07-08T02:02:01Z

flang/lib/Evaluate/tools.cpp

  return HasVectorSubscriptHelper{}(expr);
 }

+// HasTriplet()


@klausler , could you please review HasTriplet part? I'll add the other reviewers for lowering, once I add the test.

Simply checking for triplets is going to give false positives. A(:) is contiguous when A is; so are A(J:K) and A(J:K:1). There's a general contiguity checking facility in Evaluate that can handle questions of contiguity; have you tried using it?

If mustBeMadeContiguous() returns true, then this eventually gets to isSimplyContiguous() which is a helper function that calls Fortran::evaluate::IsSimplyContiguous().

And if mustBeMadeContiguous returns false, then IsSimplyContiguous is not called? Won't that lead to false negatives from e.g. A%X, non-CONTIGUOUS pointers, &c.?

That would also be a problem with the current source, without my modifications. I'll check what happens in the cases you mentioned.

klausler · 2025-07-08T15:39:46Z

flang/include/flang/Lower/CallInterface.h

    bool isIntentOut() const;
    /// Does the argument have the CONTIGUOUS attribute or have explicit shape?
-    bool mustBeMadeContiguous() const;
+    bool mustBeMadeContiguous(const bool argHasTriplet = false) const;


const is never needed in a prototype argument that isn't a reference or a pointer

Why "never"? Maybe I just want to make it obvious that the arg shouldn't be modified:

void f(const bool x = false) { x = true; } $ clang++ -c const.cpp const.cpp:3:5: error: cannot assign to variable 'x' with const-qualified type 'const bool' 3 | x = true; | ~ ^ const.cpp:1:19: note: variable 'x' declared const here 1 | void f(const bool x = false) | ~~~~~~~~~~~^~~~~~~~~ 1 error generated.

In any case, this is not important to my change, so I can remove it.

The prototype doesn't need the const here as part of the API, and the implementation is allowed to use const later if desired. So the const in the prototype makes me have to stop and wonder whether a & or * was intended but omitted.

klausler · 2025-07-08T15:45:11Z

flang/lib/Evaluate/tools.cpp

  return HasVectorSubscriptHelper{}(expr);
 }

+// HasTriplet()


Simply checking for triplets is going to give false positives. A(:) is contiguous when A is; so are A(J:K) and A(J:K:1). There's a general contiguity checking facility in Evaluate that can handle questions of contiguity; have you tried using it?

…nput, but choose to skip contiguity check

vzakhari

Thank you for the changes, Eugene! I have a couple of questions.

vzakhari · 2025-07-09T20:28:58Z

flang/lib/Lower/CallInterface.cpp

          &characteristics->u);
  if (!dummy)
    return false;
+  if (dummy->ignoreTKR.test(common::IgnoreTKR::Contiguous))


This change makes sense to me, but I am not sure about the code below.

vzakhari · 2025-07-09T20:32:57Z

flang/lib/Lower/CallInterface.cpp

+    return false;
+
+  // TODO: should this check ignore "device" or "managed"?
+  if (dummy->ignoreTKR.any() && argHasTriplet)


Why do we care about whether the actual argument is an array section or not? Can it be a non-contiguous array (e.g. assumed shape dummy) that is passed as a dummy that is declared as IGNORE_TKR?

To fix #138471, can we return the following at line 1448?

return dummy->type.Rank() > 0 || dummy->ignoreTKR.test(common::IgnoreTKR::Rank);

vzakhari · 2025-07-09T20:35:24Z

flang/test/Lower/force-temp.f90

+
+  subroutine s1()
+!CHECK-LABEL: func.func @_QMtestPs1()
+!CHECK: hlfir.copy_in


Does this test fail withtout your change? It seems that mustBeMadeContiguous should already return true for this case. I am just trying to understand whether I missed something in the compiler code or that you added this test for completeness.

That's what happens, when one oversimplifies the test. I will upload corrected test tomorrow.

eugeneepshteyn · 2025-08-18T17:41:48Z

I'll be uploading a different PR, so closing this one.

eugeneepshteyn added 7 commits July 4, 2025 16:26

[flang] Contiguity check on IGNORE_TKR argument

2477076

If argument is marked as IGNORE_TKR, enable contiguity check on it, unless IGNORE_TKR explicitly specifies to ignore contiguity.

[flang] Add -pedantic check for passing non-VOLATILE array section to…

50dd216

… VOLATILE dummy arg Add the check, which partially addresses llvm#137369 Implement HasTriplet().

Merge branch 'llvm:main' into tkr-contiguous

383bdca

Merge branch 'vol-actual-checks' into tkr-contiguous

1e76690

Undid change to definable.cpp

28cb65f

The current implementation seems to work, but I will likely move some…

a176d2d

… code around

clang-format

96daba4

eugeneepshteyn commented Jul 7, 2025

View reviewed changes

eugeneepshteyn added 3 commits July 7, 2025 19:19

Some reformatting and comments

aace0c2

clang-format

5fdfb45

Merge branch 'llvm:main' into tkr-contiguous

d105d0d

eugeneepshteyn commented Jul 8, 2025

View reviewed changes

klausler reviewed Jul 8, 2025

View reviewed changes

eugeneepshteyn added 4 commits July 9, 2025 13:51

Merge branch 'llvm:main' into tkr-contiguous

78fb202

Removed extra 'const' from the declaration

bd1e8e6

LIT test

a093fe6

Test modification: added another check for when pass non-contiguous i…

63b2811

…nput, but choose to skip contiguity check

eugeneepshteyn marked this pull request as ready for review July 9, 2025 20:12

eugeneepshteyn requested review from clementval, razvanlupusoru and vzakhari July 9, 2025 20:16

vzakhari reviewed Jul 9, 2025

View reviewed changes

eugeneepshteyn mentioned this pull request Aug 14, 2025

[flang][semantics] add portability warning and tests for copy-in/copy-out case #153263

Merged

eugeneepshteyn closed this Aug 18, 2025

eugeneepshteyn deleted the tkr-contiguous branch August 28, 2025 10:44

[flang] Create temporaries for array sections passed to IGNORE_TKR dummy args #147419

[flang] Create temporaries for array sections passed to IGNORE_TKR dummy args #147419

Uh oh!

Conversation

eugeneepshteyn commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vzakhari left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eugeneepshteyn commented Aug 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eugeneepshteyn commented Jul 7, 2025 •

edited

Loading