Skip to content
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

[flang][hlfir] Lower Cray pointee references. #65563

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

vzakhari
Copy link
Contributor

@vzakhari vzakhari commented Sep 7, 2023

A Cray pointee reference must be done using the characteristics
(bounds, type params) of the original pointee declaration, but
using the actual address value of the associated Cray pointer.
There might be multiple Cray pointees associated with the same
Cray pointer.

The proposed solution is to lower each Cray pointee into a POINTER
variable with a descriptor. The descriptor is initialized at the point
of declaration of the pointee, though its base_addr is set to null.
Before each reference of the Cray pointee its descriptor's base_addr
is updated to the current value of the Cray pointer.

The update of the base_addr is done using PointerAssociateScalar
runtime call, which just updates the base_addr of the descriptor.
This is a temporary solution just to make Cray pointers work
to the same extent they work with FIR lowering.

A Cray pointee reference must be done using the characteristics
(bounds, type params) of the original pointee declaration, but
using the actual address value of the associated Cray pointer.
There might be multiple Cray pointees associated with the same
Cray pointer.

The proposed solution is to lower each Cray pointee into a POINTER
variable with a descriptor. The descriptor is initialized at the point
of declaration of the pointee, though its base_addr is set to null.
Before each reference of the Cray pointee its descriptor's base_addr
is updated to the current value of the Cray pointer.

The update of the base_addr is done using PointerAssociateScalar
runtime call, which just updates the base_addr of the descriptor.
This is a temporary solution just to make Cray pointers work
to the same extent they work with FIR lowering.
@vzakhari vzakhari requested a review from a team as a code owner September 7, 2023 03:24
@github-actions github-actions bot added the flang Flang issues not falling into any other category label Sep 7, 2023
Copy link
Contributor

@jeanPerier jeanPerier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment, looks great, thank you Slava.

Comment on lines 1509 to 1510
mlir::Value boxAlloc =
fir::factory::genNullBoxStorage(builder, loc, ptrBoxType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code allocates and initialize the pointer storage, I think the initialization is not needed/confusing in the generated IR given it is done again with the actual shape info below.

Maybe you can just use builder.createTemporary(loc, ptrBoxType) to get the temp and move the initialization code you have below before the hlfir.declare. That way, at the point of the hlfir.declare, the pointer contains the right shape/length info.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, createTemporary should be cleaner, and I will fix it. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if moving the initialization code before hlfir.declare is necessary. Do you have an example when this may matter?

Right now the IR looks like we have a regular local pointer with the missing initialization of the pointer into an unassociated status. Since the following initialization happens before any possible use, I would say it does not matter whether it happens before or after hlfir.declare, but I may be missing something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not necessary, this was a (very minor) personal opinion regarding the readability of the generated IR. I am also fine with the way it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, okay. Thank you for the clarification!

Comment on lines 80 to 84
! CHECK: %[[VAL_25:.*]] = fir.zero_bits !fir.ptr<!fir.array<?x!fir.char<1,11>>>
! CHECK: %[[VAL_26:.*]] = arith.constant 0 : index
! CHECK: %[[VAL_27:.*]] = fir.shape %[[VAL_26]] : (index) -> !fir.shape<1>
! CHECK: %[[VAL_28:.*]] = fir.embox %[[VAL_25]](%[[VAL_27]]) : (!fir.ptr<!fir.array<?x!fir.char<1,11>>>, !fir.shape<1>) -> !fir.box<!fir.ptr<!fir.array<?x!fir.char<1,11>>>>
! CHECK: fir.store %[[VAL_28]] to %[[VAL_2]] : !fir.ref<!fir.box<!fir.ptr<!fir.array<?x!fir.char<1,11>>>>>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part that I find confusing given it is set again after the hlfir.declare with the "correct" information,

@vzakhari vzakhari merged commit f8843ef into llvm:main Sep 7, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants