This is an archive of the discontinued LLVM Phabricator instance.

[flang] Add implementation of move_alloc to the runtime
ClosedPublic

Authored by DavidTruby on Jan 9 2023, 7:34 AM.

Details

Summary

This patch adds a move_alloc implementation to the flang runtime.
Most of the checks required by the standard for move_alloc are
done by semenatic analysis; these checks are not replicated here.

Diff Detail

Event Timeline

DavidTruby created this revision.Jan 9 2023, 7:34 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
DavidTruby requested review of this revision.Jan 9 2023, 7:34 AM
klausler requested changes to this revision.Jan 9 2023, 8:53 AM
klausler added inline comments.
flang/runtime/allocatable.cpp
47

RUNTIME_CHECK() is the macro to use here.

This check will not catch a discrepancy in derived types.

You should confirm that both d'tors are allocatables.

49

Deallocate won't run finalizers (7.5.6.3 p2). You want

int stat{to.Destroy()};
if (stat != StatOk) {

return ReturnError(...);

}

57

swap() doesn't know how large the descriptors are. You want memcpy(to, from, from.SizeInBytes()) followed by from.raw.base_address = nullptr.

59

In the case of MOVE_ALLOC(x,x), it must be the case that x is in the deallocated state and nothing happens. Is that true with this implementation? If not, you'll need to check for this condition.

flang/unittests/Runtime/Allocatable.cpp
54

Braced initialization, please.

74

Other tests: ensure that FINAL procedures are called; ensure that MOVE_ALLOC(x,x) is allowed only with x in a deallocated state.

This revision now requires changes to proceed.Jan 9 2023, 8:53 AM

Use Destroy over Deallocate
Don't swap the descriptors
Add handling for MOVE_ALLOC(x,x)

DavidTruby marked 2 inline comments as done.Jan 12 2023, 5:27 AM

Thanks for the review!

flang/runtime/allocatable.cpp
47

I don't think it's possible in the general case to detect discrepancies in derived types here, because the requirement for derived types is that they're compatible, not that they're identical. This check should be done at semantic analysis, I only added the assert here as an extra precaution.

57

std::swap() just calls the assignment operator, which I performs the memcpy you are describing internally. I agree that it isn't actually necessary to do the swap though, so I've changed to just use the assignment operator once and set from to nullptr as you suggested.

flang/unittests/Runtime/Allocatable.cpp
74

I had trouble writing the test for FINAL procedures as it would require creating a derived type and associating a finalizer with it from the C++ side here; do you know if there are any other tests that do this that I can use as a reference?

Fix error string comparison check

klausler added inline comments.Jan 12 2023, 9:30 AM
flang/runtime/allocatable.cpp
46

handled*

52

Consider the case where neither allocatable is allocated.

DavidTruby added inline comments.Jan 16 2023, 6:05 AM
flang/runtime/allocatable.cpp
52

The case where neither is allocated should already fall through to the return at the end, and do nothing. I believe this is what is required in the standard.

Rebase and spelling correction

klausler accepted this revision.Jan 17 2023, 8:27 AM
klausler added inline comments.
flang/runtime/allocatable.cpp
58

if (stat != StatOk) should be used for readability so that one doesn't need to know that Fortran's successful status value is zero.

This revision is now accepted and ready to land.Jan 17 2023, 8:27 AM
This revision was landed with ongoing or failed builds.Jan 18 2023, 7:38 AM
This revision was automatically updated to reflect the committed changes.

Hello David,

I am getting the following error with -DBUILD_SHARED_LIBS=OFF build and make check-flang:

ld.lld: error: undefined symbol: mlir::SuccessorRange::SuccessorRange(mlir::Operation*)
>>> referenced by Operation.h:549 (/llvm-project/llvm/../mlir/include/mlir/IR/Operation.h:549)
>>>               CMakeFiles/FlangRuntimeTests.dir/Allocatable.cpp.o:(mlir::Operation::getSuccessors())

Hello David,

I am getting the following error with -DBUILD_SHARED_LIBS=OFF build and make check-flang:

ld.lld: error: undefined symbol: mlir::SuccessorRange::SuccessorRange(mlir::Operation*)
>>> referenced by Operation.h:549 (/llvm-project/llvm/../mlir/include/mlir/IR/Operation.h:549)
>>>               CMakeFiles/FlangRuntimeTests.dir/Allocatable.cpp.o:(mlir::Operation::getSuccessors())

D142069 seems to fix it for me.

clementval added inline comments.
flang/runtime/allocatable.cpp
46

This is not the case when one of the descriptor is polymorphic or unlimited polymorphic.
An unallocated unlimited polymorphic descriptor has not type to begin with but should be allowed here. Same with a polymorphic descriptor that is compatible.

clementval added inline comments.Jan 24 2023, 1:12 AM
flang/runtime/allocatable.cpp
46