This is an archive of the discontinued LLVM Phabricator instance.

[flang][hlfir] Do not dereference unallocated entities in structure constructor.
ClosedPublic

Authored by vzakhari on Jun 8 2023, 8:21 PM.

Details

Summary

Component-by-component assignment must be able to handle unallocated
allocatable values in structure constructor. F2018 7.5.10 p. 7 states
that the component must have unallocated status as a result of such
construction. The structure constructor temporary is initialized
such that all the allocatable components are unallocated, so we just
need to make sure not to do the component assignment if RHS is deallocated.

Depends on D152482 (the same LIT test is affected)

Diff Detail

Event Timeline

vzakhari created this revision.Jun 8 2023, 8:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2023, 8:21 PM
vzakhari requested review of this revision.Jun 8 2023, 8:21 PM
vzakhari planned changes to this revision.Jun 8 2023, 11:16 PM

Got failures in extended testing. This will need more work.

vzakhari updated this revision to Diff 530059.Jun 9 2023, 12:51 PM
  • Fixed the check for mutable RHS.
  • Fixed AssignTemporary to avoid initializing unallocated LHS.
tblah accepted this revision.Jun 12 2023, 2:29 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jun 12 2023, 2:29 AM
jeanPerier accepted this revision.Jun 13 2023, 8:59 AM

Thanks!

flang/lib/Lower/ConvertExprToHLFIR.cpp
1746

I am commenting on this patch since this is related to ALLOCATABLEs, but this is somehow disconnected: before generating the hlfir.assign op, I think you should call "loadTrivialScalars" on the RHS. We should think this through, but I feel that using pointers/allocatables directly as operands on operations where the allocatable/pointer aspect do not matter will make it harder to express their memory effects via MLIR interface since we will need to express the double read effect on the operand. It may be easier to split the deference.

On the other hand, we could just allow hlfir.assign canonicalization doing that to avoid having to think about this when generating it here and there. And maybe we can actually still easily express the double read effect.

flang/runtime/assign.cpp
570–575

I think it is likely that we end-up using AssignTemporary with an unallocated "to" for some cases of polymorphic temporaries, but it should still be OK to defer the initialization to Assign after the automatic allocation of "to".

You could comment that Assign only does the initialization of "to" if it is not already allocated, so initialization needs to be done here if only if "to" is a temporary that was already allocated (it is assumed temporaries may not be initialized yet, and it has no visible effect to initialize them again if they already have been initialized).

I am ok to defer that to whenever we actually use AssignTemporary for polymorphic temporaries.

vzakhari updated this revision to Diff 534798.Jun 26 2023, 5:39 PM
vzakhari added inline comments.
flang/lib/Lower/ConvertExprToHLFIR.cpp
1746

Thank you for the catch! Yes, loadTrivialScalar makes sense to me.

flang/runtime/assign.cpp
570–575

I modified the comment along the lines you suggested - please double check :)