Skip to content

Get the BoxType from the RHS instead of LHS for polymorphic pointer assignment inside FORALL. #164279

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 22, 2025
Merged

Get the BoxType from the RHS instead of LHS for polymorphic pointer assignment inside FORALL. #164279

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0807368
Get the BoxType from the RHS instead of LHS for polymorphic pointer a…
DanielCChen Oct 20, 2025
8949f51
To address review comments and also check if LHS is polymorphic to de…
DanielCChen Oct 21, 2025
d34dd0b
To update the LIT test and also add a new test to cover the case that…
DanielCChen Oct 22, 2025
04a9886
[NFC] Correct the description of the LIT test.
DanielCChen Oct 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions flang/lib/Lower/Bridge.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4730,13 +4730,14 @@ class FirConverter : public Fortran::lower::AbstractConverter {
if (Fortran::evaluate::UnwrapExpr<Fortran::evaluate::NullPointer>(
assign.rhs))
return fir::factory::createUnallocatedBox(*builder, loc, lhsBoxType, {});
hlfir::Entity rhs = Fortran::lower::convertExprToHLFIR(
loc, *this, assign.rhs, localSymbols, rhsContext);
hlfir::Entity rhs{fir::getBase(genExprBox(loc, assign.rhs, rhsContext))};
auto rhsBoxType =
llvm::cast<fir::BaseBoxType>(fir::unwrapRefType(rhs.getType()));
Copy link
Contributor

@jeanPerier jeanPerier Oct 21, 2025

Choose a reason for hiding this comment

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

I see why you are doing this, HLFIR is lacking a helper here.

Could you add a new method to the Entity class definition in HLFIRTools.h after getElementOrSequenceType def:

  /// Return the fir.class or fir.box type needed to describe this entity.
  fir::BaseBoxType getBoxType() const {
    if (isBoxAddressOrValue())
      return llvm::cast<fir::BaseBoxType>(fir::unwrapRefType(getType()));
    const bool isVolatile = fir::isa_volatile_type(getType());
    mlir::Type boxType =
    return fir::BoxType::get(getElementOrSequenceType(), isVolatile);
  }

That way, you can keep convertExprToHLFIR and use it to get the rhsBoxType.
Then, you should replace rhsBoxType.getBoxTypeWithNewShape(rhs.getRank()) with rhsBoxType.getBoxTypeWithNewAttr(fir::BaseBoxType::Attribute::Pointer) in the hlfir::genVariableBox to get the right attribute set via embox/rebox in the runtime descriptor that will be stored.

Copy link
Contributor Author

@DanielCChen DanielCChen Oct 21, 2025

Choose a reason for hiding this comment

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

@jeanPerier
Thanks for the quick review and the suggesion!
I tried your suggestion, but got core dump as

flang: /scratch/cdchen/FLANG/llvm-project/llvm/include/llvm/Support/Casting.h:560: decltype(auto) llvm::cast(const From &) [To = fir::BaseBoxType, From = mlir::Type]: Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' failed.

The expected rhsBoxType should be !fir.box<!fir.char<1>>, which is what the the current patch returns.
However, with convertExprToHLFIR, I got !fir.ref<!fir.char<1>>.

The code fir::getBase(genExprBox(loc, assign.rhs, rhsContext)) is also used by the exact same pointer assignment inside a DO loop (in comparison to FORALL).

Copy link
Contributor

@jeanPerier jeanPerier Oct 21, 2025

Choose a reason for hiding this comment

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

The expected rhsBoxType should be !fir.box<!fir.char<1>>, which is what the the current patch returns.
However, with convertExprToHLFIR, I got !fir.ref<!fir.char<1>>.

Yes, that is why I suggested adding the getBoxType() helper. Did you get the rhsBoxType via this new rhs.getBoxType() ?

Copy link
Contributor Author

@DanielCChen DanielCChen Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry that I mis-spoke the error.

I indeed have the correct BoxType for rhsBoxType as !fir.box<!fir.char<1>>. I also got rhsBox as

%189 = "fir.embox"(%188) <{operandSegmentSizes = array<i32: 1, 0, 0, 0, 0>}> : (!fir.ref<!fir.char<1>>) -> !fir.box<!fir.ptr<!fir.char<1>>>

However, fir.convert now complains

error: 'fir.convert' op invalid type conversion'!fir.box<!fir.ptr<!fir.char<1>>>' / '!fir.class<!fir.ptr<none>>'
error: Lowering to LLVM IR failed

Update: The error is actually from a modified code. While it is a bug, but it is not in the scope of this PR.
Sorry about the confusion. I will update the patch.

// Create pointer descriptor value from the RHS.
if (rhs.isMutableBox())
rhs = hlfir::Entity{fir::LoadOp::create(*builder, loc, rhs)};
mlir::Value rhsBox = hlfir::genVariableBox(
loc, *builder, rhs, lhsBoxType.getBoxTypeWithNewShape(rhs.getRank()));
loc, *builder, rhs, rhsBoxType.getBoxTypeWithNewShape(rhs.getRank()));
// Apply lower bounds or reshaping if any.
if (const auto *lbExprs =
std::get_if<Fortran::evaluate::Assignment::BoundsSpec>(&assign.u);
Expand Down
45 changes: 45 additions & 0 deletions flang/test/Lower/forall-polymorphic.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
! Test lower of elemental user defined assignments
! RUN: bbc -emit-fir %s -o - | FileCheck %s

! CHECK-LABEL: c.func @_QPforallpolymorphic
subroutine forallPolymorphic()
TYPE :: DT
CLASS(DT), POINTER :: Ptr(:) => NULL()
END TYPE

TYPE, EXTENDS(DT) :: DT1
END TYPE

TYPE(DT1), TARGET :: Tar1(10)
CLASS(DT), POINTER :: T(:)
integer :: I

FORALL (I=1:10)
T(I)%Ptr => Tar1
END FORALL

! CHECK: %[[V_11:[0-9]+]] = fir.alloca !fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>>> {bindc_name = "t", uniq_name = "_QFforallpolymorphicEt"}
! CHECK: %[[V_15:[0-9]+]] = fir.declare %[[V_11]] {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFforallpolymorphicEt"} : (!fir.ref<!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>>>>) -> !fir.ref<!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>>>>
! CHECK: %[[V_16:[0-9]+]] = fir.alloca !fir.array<10x!fir.type<_QFforallpolymorphicTdt1{dt:!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>}>> {bindc_name = "tar1", fir.target, uniq_name = "_QFforallpolymorphicEtar1"}
! CHECK: %[[V_17:[0-9]+]] = fir.shape %c10 : (index) -> !fir.shape<1>
! CHECK: %[[V_18:[0-9]+]] = fir.declare %[[V_16]](%[[V_17]]) {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QFforallpolymorphicEtar1"} : (!fir.ref<!fir.array<10x!fir.type<_QFforallpolymorphicTdt1{dt:!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>}>>>, !fir.shape<1>) -> !fir.ref<!fir.array<10x!fir.type<_QFforallpolymorphicTdt1{dt:!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>}>>>
! CHECK: %[[V_19:[0-9]+]] = fir.embox %[[V_18]](%[[V_17]]) : (!fir.ref<!fir.array<10x!fir.type<_QFforallpolymorphicTdt1{dt:!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>}>>>, !fir.shape<1>) -> !fir.box<!fir.array<10x!fir.type<_QFforallpolymorphicTdt1{dt:!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>}>>>
! CHECK: %[[V_34:[0-9]+]] = fir.convert %c1_i32 : (i32) -> index
! CHECK: %[[V_35:[0-9]+]] = fir.convert %c10_i32 : (i32) -> index
! CHECK: fir.do_loop %arg0 = %[[V_34]] to %[[V_35]] step %c1
! CHECK: {
! CHECK: %[[V_36:[0-9]+]] = fir.convert %arg0 : (index) -> i32
! CHECK: %[[V_37:[0-9]+]] = fir.load %[[V_15]] : !fir.ref<!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>>>>
! CHECK: %[[V_38:[0-9]+]] = fir.convert %[[V_36]] : (i32) -> i64
! CHECK: %[[C0:.*]] = arith.constant 0 : index
! CHECK: %[[V_39:[0-9]+]]:3 = fir.box_dims %37, %[[C0]] : (!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>>>, index) -> (index, index, index)
! CHECK: %[[V_40:[0-9]+]] = fir.shift %[[V_39]]#0 : (index) -> !fir.shift<1>
! CHECK: %[[V_41:[0-9]+]] = fir.array_coor %[[V_37]](%[[V_40]]) %[[V_38]] : (!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>>>, !fir.shift<1>, i64) -> !fir.ref<!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>
! CHECK: %[[V_42:[0-9]+]] = fir.embox %[[V_41]] source_box %[[V_37]] : (!fir.ref<!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>, !fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>>>) -> !fir.class<!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>
! CHECK: %[[V_43:[0-9]+]] = fir.field_index ptr, !fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>
! CHECK: %[[V_44:[0-9]+]] = fir.coordinate_of %[[V_42]], ptr : (!fir.class<!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>) -> !fir.ref<!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>>>>
! CHECK: %[[V_45:[0-9]+]] = fir.rebox %[[V_19]] : (!fir.box<!fir.array<10x!fir.type<_QFforallpolymorphicTdt1{dt:!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>}>>>) -> !fir.box<!fir.array<?x!fir.type<_QFforallpolymorphicTdt1{dt:!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>}>>>
! CHECK: %[[V_46:[0-9]+]] = fir.convert %[[V_45]] : (!fir.box<!fir.array<?x!fir.type<_QFforallpolymorphicTdt1{dt:!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>}>>>) -> !fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>>>
! CHECK: fir.store %[[V_46]] to %[[V_44]] : !fir.ref<!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt{ptr:!fir.class<!fir.ptr<!fir.array<?x!fir.type<_QFforallpolymorphicTdt>>>>}>>>>>
Copy link
Contributor

@jeanPerier jeanPerier Oct 21, 2025

Choose a reason for hiding this comment

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

This will end-up erasing the pointer attribute in the runtime descriptor for the pointers because it was not set in the descriptor value that V_45, and convert is a reinterpret_cast for descriptor (it does not change/ensure that the runtime attributes match the type attributes).
Suggestion fix in code.

! CHECK: }
end subroutine forallPolymorphic