-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[flang] implement show_descriptor intrinsic, a non-standard extension #169137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Output examples: |
🐧 Linux x64 Test Results
|
| } | ||
|
|
||
| void Descriptor::Dump(FILE *f) const { | ||
| static const char *GetTypeStr(ISO::CFI_type_t type, bool dumpRawType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringRef instead if const char*?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using llvm::StringRef would be a precedent for flang-rt.
vzakhari
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, Valery! It looks good to me.
I have one question inlined. Can you please also add unit tests into flang-rt/unittests/Runtime/?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this, just some high level feedback:
I think I would slightly favor it being defined in an interface in some new intrinsic flang_debug module for better control/error messages when people are switching between compilers (@klausler for opinions).
I understand that defining it fully via Fortran without some interception in semantics is likely hard (probably require some combo of IGNORE_TKR).
However, maybe defining it via renaming to a builtin is an option (see how this is done for __builtin_c_loc for instance).
Second point is that POINTER/ALLOCATABLE descriptors will print the address of a copy (because of the fir.load) and I think it may be useful to get the real address for those. However, this can easily be improved later by updating lowering (adding an entry point if needed to satisfy FIR type between fir.box and fir.ref<fir.box>) and is not a blocking point.
|
This needs to be documented in https://github.com/llvm/llvm-project/blob/main/flang/docs/Intrinsics.md |
Add flang_debug module to allows using show_descriptor name by a user.
|
Thank you for all your inputs. I tried to address all the raised concerns (except handling pointer/allocatable, which I'd like to do in a follow up patch). |
vzakhari
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the tests, Valery!
It LGTM, but please let Jean review the lowering part.
jeanPerier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit on the test, LGTM otherwise, thanks for the update.
| ! CHECK: %[[LOAD_1:.*]] = fir.load %[[DECLARE_0]] : !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>> | ||
| ! CHECK: fir.call @_FortranAShowDescriptor(%[[LOAD_1]]) fastmath<contract> : (!fir.box<!fir.heap<!fir.array<?xi32>>>) -> () | ||
|
|
||
| call __builtin_show_descriptor(a(1:3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the test to do use flang_debug and use show_descriptor?
User should not use the flang __builtin directly, and it would be good if the test also exercise the new module (I think bcc should pick the intrinsic modules, if it does not you can update the test to use flang_fc1).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks
show_descriptor intrinsic prints details of a descriptor (extended Fortran pointer).
It accepts a descriptor for any type and rank, including scalars.
Requires use of flang_debug module.
Example:
program test
use flang_debug
implicit none
integer :: a(4) = (/ 1,3,5,7 /)
call show_descriptor(a(1:3))
end program test
and its output:
Descriptor @ 0x7ffe01ec6a98:
base_addr 0x563b7035103c
elem_len 4
version 20240719
rank 1
type 9 "INTEGER(kind=4)"
attribute 0
extra 0
addendum 0
alloc_idx 0
dim[0] lower_bound 1
extent 3
sm 4