This is an archive of the discontinued LLVM Phabricator instance.

[fir] Add fir.box_isarray, fir.box_isptr and fir.box_isalloc conversion.
ClosedPublic

Authored by clementval on Nov 10 2021, 7:13 AM.

Details

Summary

This patch is part of the upstreaming effort from fir-dev branch.

Co-authored-by: Eric Schweitz <eschweitz@nvidia.com>

Diff Detail

Event Timeline

clementval created this revision.Nov 10 2021, 7:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 7:13 AM
Herald added a subscriber: mehdi_amini. · View Herald Transcript
clementval requested review of this revision.Nov 10 2021, 7:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 7:13 AM
awarzynski added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
34–35

Why not use the macro directly?

289
clementval added inline comments.Nov 10 2021, 8:22 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
34–35

fir.box is an abstraction of a descriptor also we can get a compile time error in case the macro is not a uint.

clementval added inline comments.Nov 10 2021, 8:23 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
34–35

also it makes it homogenous with the fir.box field indexing added previously so the code is maybe easier to understand.

Makes sense, left a few minor comments/nits.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
33

i.e. CFI_attribute_t?

34–35

we can get a compile time error in case the macro is not a uint

Any idea why wouldn't the original value be define as uint? That would make it easier for you here.

also it makes it homogenous with the fir.box field indexing added previously so the code is maybe easier to understand.

Agreed, this does help a lot!

I think that you are doing a great job and a massive favour to all of us here. I'm just thinking that perhaps stuff from "flang/ISO_Fortran_binding.h" should be wrapped in an enum class or something similar? Basically, sth to facilitate code re-use a bit better. WDYT?

125–127

DOXYGENME

IMHO, this comment is a bit unclear. How about:

Creates a sequence of instructions to load the box attribute from \p box and to check it against  \p maskValue. The final check is implemented as `(attribute && maskValue) != 0`.

Or something similar?

270–271

[nit] This is the case just now, but might not be in the future. Also, the two implementations are independent (i.e. there's no code re-use). So I would delete this. Only the first sentence is key.

flang/test/Fir/convert-to-llvm.fir
769

[nit] Perhaps add that it will load the rank and comparer it against 0?

788

[nit] Perhaps add that it will load the attribute and check it against the mask corresponding to CFI_attribute_allocatable?

799

Make this platform agnostic?

809

[nit] Perhaps add that it will load the attribute and check it against the mask corresponding to CFI_attribute_pointer?

820

Make this platform agnostic?

clementval marked 11 inline comments as done.

Address comments

awarzynski accepted this revision.Nov 11 2021, 1:28 AM

LGTM, thank you for addressing my comments!

flang/test/Fir/convert-to-llvm.fir
790
813
This revision is now accepted and ready to land.Nov 11 2021, 1:28 AM
clementval marked 2 inline comments as done.Nov 11 2021, 1:46 AM
clementval added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
34–35

we can get a compile time error in case the macro is not a uint

Any idea why wouldn't the original value be define as uint? That would make it easier for you here.

Who knows what people can do.

also it makes it homogenous with the fir.box field indexing added previously so the code is maybe easier to understand.

Agreed, this does help a lot!

I think that you are doing a great job and a massive favour to all of us here. I'm just thinking that perhaps stuff from "flang/ISO_Fortran_binding.h" should be wrapped in an enum class or something similar? Basically, sth to facilitate code re-use a bit better. WDYT?

This could be an improvement but it's not something that's in the scope of this patch and will likely require some effort in fir-dev first.