This is an archive of the discontinued LLVM Phabricator instance.

[flang] Lower TYPE(*) as fir.box<none>
ClosedPublic

Authored by clementval on Oct 4 2022, 4:28 AM.

Details

Summary

This patch lowers TYPE(*) correctly to fir.box<none>.

Diff Detail

Event Timeline

clementval created this revision.Oct 4 2022, 4:28 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 4 2022, 4:28 AM
Herald added a subscriber: mehdi_amini. · View Herald Transcript
clementval requested review of this revision.Oct 4 2022, 4:28 AM
clementval updated this revision to Diff 464955.Oct 4 2022, 4:31 AM

Remove obsolete comment

Looking at the design doc, I do not remember if you have considered using fir.box<None> for TYPE(*). Since Type(*) usage is quite limited unlike class(), I do not know if it is really worth encoding it as a special class.
In fact, we are sort of already using fir.box<none> as a TYPE(*) equivalent around the runtime interface. So unless you see specific reasons to use and extend fir.class here, maybe using fir.box would be enough ?

Looking at the design doc, I do not remember if you have considered using fir.box<None> for TYPE(*). Since Type(*) usage is quite limited unlike class(), I do not know if it is really worth encoding it as a special class.
In fact, we are sort of already using fir.box<none> as a TYPE(*) equivalent around the runtime interface. So unless you see specific reasons to use and extend fir.class here, maybe using fir.box would be enough ?

Yes we can also use fir.type<none> for TYPE(*). At the moment, I do not see any specific reasons we need to distinguish between the current usage of fir.box<none> and TYPE(*) but of course we can always change that in case a reason arise. The motivation here was to keep all polymorphic entities represented as fir.class and since TYPE(*) is unlimited polymorphic then it would have fall in that representation as well. I'm fine with going with fir.box<none>. It would also remove the need of a custom assembly format.

Looking at the design doc, I do not remember if you have considered using fir.box<None> for TYPE(*). Since Type(*) usage is quite limited unlike class(), I do not know if it is really worth encoding it as a special class.
In fact, we are sort of already using fir.box<none> as a TYPE(*) equivalent around the runtime interface. So unless you see specific reasons to use and extend fir.class here, maybe using fir.box would be enough ?

Yes we can also use fir.type<none> for TYPE(*). At the moment, I do not see any specific reasons we need to distinguish between the current usage of fir.box<none> and TYPE(*) but of course we can always change that in case a reason arise. The motivation here was to keep all polymorphic entities represented as fir.class and since TYPE(*) is unlimited polymorphic then it would have fall in that representation as well. I'm fine with going with fir.box<none>. It would also remove the need of a custom assembly format.

OK, then I vote for making TYPE(*) be fir.box<none>. Given C709 restrictions about assumed types, I think it makes more sense to keep them separate from CLASS entities and reserve fir.class for CLASS.

Looking at the design doc, I do not remember if you have considered using fir.box<None> for TYPE(*). Since Type(*) usage is quite limited unlike class(), I do not know if it is really worth encoding it as a special class.
In fact, we are sort of already using fir.box<none> as a TYPE(*) equivalent around the runtime interface. So unless you see specific reasons to use and extend fir.class here, maybe using fir.box would be enough ?

Yes we can also use fir.type<none> for TYPE(*). At the moment, I do not see any specific reasons we need to distinguish between the current usage of fir.box<none> and TYPE(*) but of course we can always change that in case a reason arise. The motivation here was to keep all polymorphic entities represented as fir.class and since TYPE(*) is unlimited polymorphic then it would have fall in that representation as well. I'm fine with going with fir.box<none>. It would also remove the need of a custom assembly format.

OK, then I vote for making TYPE(*) be fir.box<none>. Given C709 restrictions about assumed types, I think it makes more sense to keep them separate from CLASS entities and reserve fir.class for CLASS.

Ok fine for me. I'll update the patch and the other one today.

clementval updated this revision to Diff 464996.Oct 4 2022, 6:52 AM

Switch to fir.box<none> representation

clementval retitled this revision from [flang] Lower TYPE(*) as fir.class<none, assumed_type> to [flang] Lower TYPE(*) as fir.box<none>.Oct 4 2022, 6:53 AM
clementval edited the summary of this revision. (Show Details)
jeanPerier accepted this revision.Oct 4 2022, 8:05 AM

LGTM, thanks

This revision is now accepted and ready to land.Oct 4 2022, 8:05 AM
jeanPerier added inline comments.Oct 4 2022, 8:07 AM
flang/lib/Lower/CallInterface.cpp
862–863

Beware builbot clang-format is complaining for this file.

clang-format

clementval marked an inline comment as done.Oct 4 2022, 11:27 AM
clementval added inline comments.
flang/lib/Lower/CallInterface.cpp
862–863

Thanks. I had a bad clang-format config in arcanist for a while.

This revision was automatically updated to reflect the committed changes.
clementval marked an inline comment as done.