[flang] Lower PRIVATE component names safely #66076
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-flang-fir-hlfir ChangesIt is possible for a derived type extending a type with private components to define components with the same name as the private components. This was not properly handled by lowering where several fir.record type component names could end-up being the same, leading to bad generated code (only the first component was accessed via fir.field_index, leading to bad generated code). This patch handles the situation by adding the derived type mangled name to private component.Patch is 35.98 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/66076.diff 13 Files Affected:
diff --git a/flang/include/flang/Lower/AbstractConverter.h b/flang/include/flang/Lower/AbstractConverter.h
index 1e2d2fb6fa60d72..477c8164a18eafd 100644
--- a/flang/include/flang/Lower/AbstractConverter.h
+++ b/flang/include/flang/Lower/AbstractConverter.h
@@ -250,6 +250,11 @@ class AbstractConverter {
mangleName(const Fortran::semantics::DerivedTypeSpec &) = 0;
/// Unique a compiler generated name (add a containing scope specific prefix)
virtual std::string mangleName(std::string &) = 0;
+ /// Return the field name for a derived type component inside a fir.record
+ /// type.
+ virtual std::string
+ getRecordTypeFieldName(const Fortran::semantics::Symbol &component) = 0;
+
/// Get the KindMap.
virtual const fir::KindMapping &getKindMap() = 0;
diff --git a/flang/include/flang/Lower/Mangler.h b/flang/include/flang/Lower/Mangler.h
index 9eb4e3e853a9e48..41939abe29e5e22 100644
--- a/flang/include/flang/Lower/Mangler.h
+++ b/flang/include/flang/Lower/Mangler.h
@@ -96,6 +96,13 @@ inline std::string mangleArrayLiteral(
/// Return the compiler-generated name of a static namelist variable descriptor.
std::string globalNamelistDescriptorName(const Fortran::semantics::Symbol &sym);
+/// Return the field name for a derived type component inside a fir.record type.
+/// It is the component name if the component is not private. Otherwise it is
+/// mangled with the component parent type to avoid any name clashes in type
+/// extensions.
+std::string getRecordTypeFieldName(const Fortran::semantics::Symbol &component,
+ ScopeBlockIdMap &);
+
} // namespace lower::mangle
} // namespace Fortran
diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index cbe108194dd212f..ec166628f47b159 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -848,6 +848,11 @@ class FirConverter : public Fortran::lower::AbstractConverter {
return Fortran::lower::mangle::mangleName(name, getCurrentScope(),
scopeBlockIdMap);
}
+ std::string getRecordTypeFieldName(
+ const Fortran::semantics::Symbol &component) override final {
+ return Fortran::lower::mangle::getRecordTypeFieldName(component,
+ scopeBlockIdMap);
+ }
const fir::KindMapping &getKindMap() override final {
return bridge.getKindMap();
}
diff --git a/flang/lib/Lower/ConvertConstant.cpp b/flang/lib/Lower/ConvertConstant.cpp
index d1887b610c8fae8..ded0a1959a6c1eb 100644
--- a/flang/lib/Lower/ConvertConstant.cpp
+++ b/flang/lib/Lower/ConvertConstant.cpp
@@ -362,8 +362,9 @@ static mlir::Value genInlinedStructureCtorLitImpl(
if (sym->test(Fortran::semantics::Symbol::Flag::ParentComp))
TODO(loc, "parent component in structure constructor");
- llvm::StringRef name = toStringRef(sym->name());
+ std::string name = converter.getRecordTypeFieldName(sym);
mlir::Type componentTy = recTy.getType(name);
+ assert(componentTy && "failed to retrieve component");
// FIXME: type parameters must come from the derived-type-spec
auto field = builder.create(
loc, fieldTy, name, type,
diff --git a/flang/lib/Lower/ConvertExpr.cpp b/flang/lib/Lower/ConvertExpr.cpp
index a9298be5532d905..7507b7aa94dc8e1 100644
--- a/flang/lib/Lower/ConvertExpr.cpp
+++ b/flang/lib/Lower/ConvertExpr.cpp
@@ -937,7 +937,7 @@ class ScalarExprLowering {
if (isDerivedTypeWithLenParameters(sym))
TODO(loc, "component with length parameters in structure constructor");
- llvm::StringRef name = toStringRef(sym.name());
+ std::string name = converter.getRecordTypeFieldName(sym);
// FIXME: type parameters must come from the derived-type-spec
mlir::Value field = builder.create(
loc, fieldTy, name, ty,
@@ -1476,7 +1476,7 @@ class ScalarExprLowering {
for (const Fortran::evaluate::Component *field : list) {
auto recTy = ty.cast();
const Fortran::semantics::Symbol &sym = getLastSym(*field);
- llvm::StringRef name = toStringRef(sym.name());
+ std::string name = converter.getRecordTypeFieldName(sym);
coorArgs.push_back(builder.create(
loc, fldTy, name, recTy, fir::getTypeParams(obj)));
ty = recTy.getType(name);
@@ -6745,7 +6745,8 @@ class ArrayExprLowering {
},
[&](const Fortran::evaluate::Component *x) {
auto fieldTy = fir::FieldType::get(builder.getContext());
- llvm::StringRef name = toStringRef(getLastSym(*x).name());
+ std::string name =
+ converter.getRecordTypeFieldName(getLastSym(*x));
if (auto recTy = ty.dyn_cast()) {
ty = recTy.getType(name);
auto fld = builder.create(
diff --git a/flang/lib/Lower/ConvertExprToHLFIR.cpp b/flang/lib/Lower/ConvertExprToHLFIR.cpp
index ee4d307b2afbd7b..bc98fdd917d41d0 100644
--- a/flang/lib/Lower/ConvertExprToHLFIR.cpp
+++ b/flang/lib/Lower/ConvertExprToHLFIR.cpp
@@ -742,7 +742,7 @@ class HlfirDesignatorBuilder {
assert(
!componentSym.test(Fortran::semantics::Symbol::Flag::ParentComp) &&
"parent components are skipped and must not reach visitComponentImpl");
- partInfo.componentName = componentSym.name().ToString();
+ partInfo.componentName = converter.getRecordTypeFieldName(componentSym);
auto recordType =
hlfir::getFortranElementType(baseType).cast();
if (recordType.isDependentType())
@@ -1721,7 +1721,7 @@ class HlfirBuilder {
for (const auto &value : ctor.values()) {
const Fortran::semantics::Symbol &sym = *value.first;
const Fortran::lower::SomeExpr &expr = value.second.value();
- llvm::StringRef name = toStringRef(sym.name());
+ std::string name = converter.getRecordTypeFieldName(sym);
if (sym.test(Fortran::semantics::Symbol::Flag::ParentComp)) {
const Fortran::semantics::DeclTypeSpec *declTypeSpec = sym.GetType();
assert(declTypeSpec && declTypeSpec->AsDerived() &&
diff --git a/flang/lib/Lower/ConvertType.cpp b/flang/lib/Lower/ConvertType.cpp
index e8179b43afc7d11..bfddf9a15084c85 100644
--- a/flang/lib/Lower/ConvertType.cpp
+++ b/flang/lib/Lower/ConvertType.cpp
@@ -382,23 +382,25 @@ struct TypeBuilderImpl {
// Gather the record type fields.
// (1) The data components.
- for (const auto &field :
+ for (const auto &component :
Fortran::semantics::OrderedComponentIterator(tySpec)) {
// Lowering is assuming non deferred component lower bounds are always 1.
// Catch any situations where this is not true for now.
if (!converter.getLoweringOptions().getLowerToHighLevelFIR() &&
- componentHasNonDefaultLowerBounds(field))
- TODO(converter.genLocation(field.name()),
+ componentHasNonDefaultLowerBounds(component))
+ TODO(converter.genLocation(component.name()),
"derived type components with non default lower bounds");
- if (IsProcedure(field))
- TODO(converter.genLocation(field.name()), "procedure components");
- mlir::Type ty = genSymbolType(field);
+ if (IsProcedure(component))
+ TODO(converter.genLocation(component.name()), "procedure components");
+ mlir::Type ty = genSymbolType(component);
// Do not add the parent component (component of the parents are
// added and should be sufficient, the parent component would
- // duplicate the fields).
- if (field.test(Fortran::semantics::Symbol::Flag::ParentComp))
+ // duplicate the fields). Note that genSymbolType must be called above on
+ // it so that the dispatch table for the parent type still gets emitted
+ // as needed.
+ if (component.test(Fortran::semantics::Symbol::Flag::ParentComp))
continue;
- cs.emplace_back(field.name().ToString(), ty);
+ cs.emplace_back(converter.getRecordTypeFieldName(component), ty);
}
// (2) The LEN type parameters.
diff --git a/flang/lib/Lower/Mangler.cpp b/flang/lib/Lower/Mangler.cpp
index 8e94ccfa70498e3..d904d6038c3e2c8 100644
--- a/flang/lib/Lower/Mangler.cpp
+++ b/flang/lib/Lower/Mangler.cpp
@@ -230,6 +230,25 @@ std::string Fortran::lower::mangle::mangleName(
return fir::NameUniquer::doType(modules, procs, blockId, symbolName, kinds);
}
+std::string Fortran::lower::mangle::getRecordTypeFieldName(
+ const Fortran::semantics::Symbol &component,
+ ScopeBlockIdMap &scopeBlockIdMap) {
+ if (!component.attrs().test(Fortran::semantics::Attr::PRIVATE))
+ return component.name().ToString();
+ const Fortran::semantics::DerivedTypeSpec *componentParentType =
+ component.owner().derivedTypeSpec();
+ assert(componentParentType &&
+ "failed to retrieve private component parent type");
+ // Do not mangle Iso C C_PTR and C_FUNPTR components. This type cannot be
+ // extended as per Fortran 2018 7.5.7.1, mangling them makes the IR unreadable
+ // when using ISO C modules, and lowering needs to know the component way
+ // without access to semantics::Symbol.
+ if (Fortran::semantics::IsIsoCType(componentParentType))
+ return component.name().ToString();
+ return component.name().ToString() + "." +
+ mangleName(*componentParentType, scopeBlockIdMap);
+}
+
std::string Fortran::lower::mangle::demangleName(llvm::StringRef name) {
auto result = fir::NameUniquer::deconstruct(name);
return result.second.name;
diff --git a/flang/test/Lower/HLFIR/private-components.f90 b/flang/test/Lower/HLFIR/private-components.f90
new file mode 100644
index 000000000000000..d8d7b1c6a791ce1
--- /dev/null
+++ b/flang/test/Lower/HLFIR/private-components.f90
@@ -0,0 +1,35 @@
+! Test that private component names are mangled inside fir.record
+! in a way that allow components with the same name to be added in
+! type extensions.
+! RUN: bbc -emit-hlfir -o - %s | FileCheck %s
+
+module name_clash
+ type:: t
+ integer, private :: i
+ end type
+end module
+
+!CHECK-LABEL: func.func @_QPuser_clash(
+!CHECK-SAME: !fir.ref>
+!CHECK-SAME: !fir.ref>
+subroutine user_clash(a, at)
+ use name_clash
+ type,extends(t) :: t2
+ integer :: i = 2
+ end type
+ type(t2) :: a, b
+ type(t) :: at
+ print *, a%i
+ print *, t2(t=at)
+ a = b
+end subroutine
+
+! CHECK-LABEL: func.func @_QPclash_with_intrinsic_module(
+! CHECK-SAME: !fir.ref>
+subroutine clash_with_intrinsic_module(a)
+ use ieee_arithmetic
+ type, extends(ieee_class_type) :: my_class
+ integer(1) :: which
+ end type
+ type(my_class) :: a
+end subroutine
diff --git a/flang/test/Lower/Intrinsics/ieee_class.f90 b/flang/test/Lower/Intrinsics/ieee_class.f90
index b003284b4b5f8b6..a121fb7c73575a2 100644
--- a/flang/test/Lower/Intrinsics/ieee_class.f90
+++ b/flang/test/Lower/Intrinsics/ieee_class.f90
@@ -41,8 +41,8 @@ subroutine classify(x)
use m; use ieee_arithmetic
real(k) :: x
! CHECK-DAG: %[[V_0:[0-9]+]] = fir.alloca i32 {adapt.valuebyref}
- ! CHECK-DAG: %[[V_1:[0-9]+]] = fir.alloca !fir.type<_QMieee_arithmeticTieee_class_type{which:i8}>
- ! CHECK-DAG: %[[V_2:[0-9]+]] = fir.alloca !fir.type<_QMieee_arithmeticTieee_class_type{which:i8}> {bindc_name = "r", uniq_name = "_QFclassifyEr"}
+ ! CHECK-DAG: %[[V_1:[0-9]+]] = fir.alloca !fir.type<_QMieee_arithmeticTieee_class_type{which._QMieee_arithmeticTieee_class_type:i8}>
+ ! CHECK-DAG: %[[V_2:[0-9]+]] = fir.alloca !fir.type<_QMieee_arithmeticTieee_class_type{which._QMieee_arithmeticTieee_class_type:i8}> {bindc_name = "r", uniq_name = "_QFclassifyEr"}
type(ieee_class_type) :: r
! CHECK: %[[V_8:[0-9]+]] = fir.load %arg0 : !fir.ref
@@ -64,24 +64,24 @@ subroutine classify(x)
! CHECK: %[[V_24:[0-9]+]] = arith.andi %[[V_23]], %c1{{.*}} : i64
! CHECK: %[[V_25:[0-9]+]] = arith.ori %[[V_22]], %[[V_24]] : i64
! CHECK: %[[V_26:[0-9]+]] = fir.address_of(@_FortranAIeeeClassTable) : !fir.ref>
- ! CHECK: %[[V_27:[0-9]+]] = fir.coordinate_of %[[V_26]], %[[V_25]] : (!fir.ref>, i64) -> !fir.ref>
- ! CHECK: %[[V_28:[0-9]+]] = fir.field_index which, !fir.type<_QMieee_arithmeticTieee_class_type{which:i8}>
- ! CHECK: %[[V_29:[0-9]+]] = fir.coordinate_of %[[V_27]], %[[V_28]] : (!fir.ref>, !fir.field) -> !fir.ref
- ! CHECK: %[[V_30:[0-9]+]] = fir.field_index which, !fir.type<_QMieee_arithmeticTieee_class_type{which:i8}>
- ! CHECK: %[[V_31:[0-9]+]] = fir.coordinate_of %[[V_2]], %[[V_30]] : (!fir.ref>, !fir.field) -> !fir.ref
+ ! CHECK: %[[V_27:[0-9]+]] = fir.coordinate_of %[[V_26]], %[[V_25]] : (!fir.ref>, i64) -> !fir.ref>
+ ! CHECK: %[[V_28:[0-9]+]] = fir.field_index which._QMieee_arithmeticTieee_class_type, !fir.type<_QMieee_arithmeticTieee_class_type{which._QMieee_arithmeticTieee_class_type:i8}>
+ ! CHECK: %[[V_29:[0-9]+]] = fir.coordinate_of %[[V_27]], %[[V_28]] : (!fir.ref>, !fir.field) -> !fir.ref
+ ! CHECK: %[[V_30:[0-9]+]] = fir.field_index which._QMieee_arithmeticTieee_class_type, !fir.type<_QMieee_arithmeticTieee_class_type{which._QMieee_arithmeticTieee_class_type:i8}>
+ ! CHECK: %[[V_31:[0-9]+]] = fir.coordinate_of %[[V_2]], %[[V_30]] : (!fir.ref>, !fir.field) -> !fir.ref
! CHECK: %[[V_32:[0-9]+]] = fir.load %[[V_29]] : !fir.ref
! CHECK: fir.store %[[V_32]] to %[[V_31]] : !fir.ref
r = ieee_class(x)
! if (r==ieee_signaling_nan) call out(x, 1)
! if (r==ieee_quiet_nan) call out(x, 2)
- ! CHECK: %[[V_38:[0-9]+]] = fir.field_index which, !fir.type<_QMieee_arithmeticTieee_class_type{which:i8}>
- ! CHECK: %[[V_39:[0-9]+]] = fir.coordinate_of %[[V_1]], %[[V_38]] : (!fir.ref>, !fir.field) -> !fir.ref
+ ! CHECK: %[[V_38:[0-9]+]] = fir.field_index which._QMieee_arithmeticTieee_class_type, !fir.type<_QMieee_arithmeticTieee_class_type{which._QMieee_arithmeticTieee_class_type:i8}>
+ ! CHECK: %[[V_39:[0-9]+]] = fir.coordinate_of %[[V_1]], %[[V_38]] : (!fir.ref>, !fir.field) -> !fir.ref
! CHECK: fir.store %c3{{.*}} to %[[V_39]] : !fir.ref
- ! CHECK: %[[V_40:[0-9]+]] = fir.field_index which, !fir.type<_QMieee_arithmeticTieee_class_type{which:i8}>
- ! CHECK: %[[V_41:[0-9]+]] = fir.coordinate_of %[[V_2]], %[[V_40]] : (!fir.ref>, !fir.field) -> !fir.ref
- ! CHECK: %[[V_42:[0-9]+]] = fir.field_index which, !fir.type<_QMieee_arithmeticTieee_class_type{which:i8}>
- ! CHECK: %[[V_43:[0-9]+]] = fir.coordinate_of %[[V_1]], %[[V_42]] : (!fir.ref>, !fir.field) -> !fir.ref
+ ! CHECK: %[[V_40:[0-9]+]] = fir.field_index which._QMieee_arithmeticTieee_class_type, !fir.type<_QMieee_arithmeticTieee_class_type{which._QMieee_arithmeticTieee_class_type:i8}>
+ ! CHECK: %[[V_41:[0-9]+]] = fir.coordinate_of %[[V_2]], %[[V_40]] : (!fir.ref>, !fir.field) -> !fir.ref
+ ! CHECK: %[[V_42:[0-9]+]] = fir.field_index which._QMieee_arithmeticTieee_class_type, !fir.type<_QMieee_arithmeticTieee_class_type{which._QMieee_arithmeticTieee_class_type:i8}>
+ ! CHECK: %[[V_43:[0-9]+]] = fir.coordinate_of %[[V_1]], %[[V_42]] : (!fir.ref>, !fir.field) -> !fir.ref
! CHECK: %[[V_44:[0-9]+]] = fir.load %[[V_41]] : !fir.ref
! CHECK: %[[V_45:[0-9]+]] = fir.load %[[V_43]] : !fir.ref
! CHECK: %[[V_46:[0-9]+]] = arith.cmpi eq, %[[V_44]], %[[V_45]] : i8
@@ -109,13 +109,13 @@ program p
! x(1) = ieee_value(x(1), ieee_signaling_nan)
! x(2) = ieee_value(x(1), ieee_quiet_nan)
- ! CHECK: %[[V_0:[0-9]+]] = fir.alloca !fir.type<_QMieee_arithmeticTieee_class_type{which:i8}>
+ ! CHECK: %[[V_0:[0-9]+]] = fir.alloca !fir.type<_QMieee_arithmeticTieee_class_type{which._QMieee_arithmeticTieee_class_type:i8}>
! CHECK: %[[V_2:[0-9]+]] = fir.address_of(@_QFEx) : !fir.ref>
- ! CHECK: %[[V_8:[0-9]+]] = fir.field_index which, !fir.type<_QMieee_arithmeticTieee_class_type{which:i8}>
- ! CHECK: %[[V_9:[0-9]+]] = fir.coordinate_of %[[V_0]], %[[V_8]] : (!fir.ref>, !fir.field) -> !fir.ref
+ ! CHECK: %[[V_8:[0-9]+]] = fir.field_index which._QMieee_arithmeticTieee_class_type, !fir.type<_QMieee_arithmeticTieee_class_type{which._QMieee_arithmeticTieee_class_type:i8}>
+ ! CHECK: %[[V_9:[0-9]+]] = fir.coordinate_of %[[V_0]], %[[V_8]] : (!fir.ref>, !fir.field) -> !fir.ref
! CHECK: fir.store %c3{{.*}} to %[[V_9]] : !fir.ref
- ! CHECK: %[[V_10:[0-9]+]] = fir.field_index which, !fir.type<_QMieee_arithmeticTieee_class_type{which:i8}>
- ! CHECK: %[[V_11:[0-9]+]] = fir.coordinate_of %[[V_0]], %[[V_10]] : (!fir.ref>, !fir.field) -> !fir.ref
+ ! CHECK: %[[V_10:[0-9]+]] = fir.field_index which._QMieee_arithmeticTieee_class_type, !fir.type<_QMieee_arithmeticTieee_class_type{which._QMieee_arithmeticTieee_class_type:i8}>
+ ! CHECK: %[[V_11:[0-9]+]] = fir.coordinate_of %[[V_0]], %[[V_10]] : (!fir.ref>, !fir.field) -> !fir.ref
! CHECK: %[[V_12:[0-9]+]] = fir.load %[[V_11]] : !fir.ref
! CHECK: %[[V_13:[0-9]+]] = fir.address_of(@_FortranAIeeeValueTable_8) : !fir.ref>
! CHECK: %[[V_14:[0-9]+]] = fir.coordinate_of %[[V_13]], %[[V_12]] : (!fir.ref>, i8) -> !fir.ref
diff --git a/flang/test/Lower/Intrinsics/ieee_operator_eq.f90 b/flang/test/Lower/Intrinsics/ieee_operator_eq.f90
index 4c2f7271cd22805..3bf2c7ea15d702a 100644
--- a/flang/test/Lower/Intrinsics/ieee_operator_eq.f90
+++ b/flang/test/Lower/Intrinsics/ieee_operator_eq.f90
@@ -4,10 +4,10 @@
subroutine s(r1,r2)
use ieee_arithmetic, only: ieee_round_type, operator(==)
type(ieee_round_type) :: r1, r2
- ! CHECK: %[[V_3:[0-9]+]] = fir.field_index mode, !fir.type<_QMieee_arithmeticTieee_round_type{mode:i8}>
- ! CHECK: %[[V_4:[0-9]+]] = fir.coordinate_of %arg0, %[[V_3]] : (!fir.ref>, !fir.field) -> !fir.ref
- ! CHECK: %[[V_5:[0-9]+]] = fir.field_index mode, !fir.type<_QMieee_arithmeticTieee_round_type{mode:i8}>
- ! CHECK: %[[V_6:[0-9]+]] = fir.coordinate_of %arg1, %[[V_5]] : (!fir.ref
|
psteinfeld
approved these changes
Sep 12, 2023
|
This change will make IR harder to read. It's a somewhat heavy weight fix for a minor issue. I'm not sure I understand the details, such as why PRIVATE is relevant, but it would be nice if there were a lighter weight fix for this. Is it possible to have a 3-way or a 4-way component name conflict? (If this is limited to a 2-way conflict, then it might be better to mangle the (unique?) child name.) But if I'm understanding this to some degree, you really can't put the mangling on the child component. Mangling must be proactively applied to the parent component, just in case there are one or more extensions that introduce conflicting names, possibly in a different file. (If not, it seems like it would be better to mangle child names.) Would it be possible to solve the problem by always adding a prefix or suffix tag to private components? Something like Or can some kind of numeric "extension depth" tag be used to a differentiate names and avoid conflicts? Another possibility would be to put more effort into checking for a conflict, if separate compilation doesn't make that impossible or impractical. That's also kind of heavy weight, but the cost is compile time rather than changing the IR. If nothing else helps to avoid adding type name tags, I think it would be better to swap the order and put the containing type name first. |
For private components, yes. It is a trade off I chose over more complex compiler code and more time to find the name of "normal" components. It is an easy rule to apply.
You can have any number of components with the same name:
From an IR readability, I agree, the problem with this approach is that every time you want to know a field name you will then need a linear search over the other type components to look for conflict.
No, see example above, you can have any number of component conflicting.
Yes but at compile time cost for a lot of regular types with no private components.
What I do not like is that the cost of this analysis will be paid by every derived type, even if they do not have private components (since you cannot know that until you have looked at all the other components).
There is a "cheaper" approach that would keep the IR lighter, but I am not a big fan. It is to mangle private components only when they appear in an extension of the base type containing the component. There is no extra cost for non private component (protected by a simple Private attribute check). However, this means the private components will be mangled differently in the base and extended type, so it makes it harder to make "connections" between the components of a type and the components of this extensions... (let say risky, people would mostly not think of this case an just assume the name must be the same). You would basically have My take from our tests is that private components are not so common so that it is worth increasing complexity for the rest and it is not worth breaking the easy to work with predicate that component of some base type always have the same FIR field name in the type extensions. Regarding the ugliness of the IR, we have a long due task to use MLIR type alias to improve the IR readability (the full !_QMsomemodTsometype = fir.type<.....> would only be defined once and then you would directly see fir.ref<!_QMsomemodTsometype> in the IR without the whole list of components every time the types appear). Would you be OK with the current approach given the possibility of mlir type aliasing? |
|
Ok, thanks for considering alternatives. It should be ok to take this approach, at least for now. We can reconsider later if it might be worthwhile to look at other alternatives such as making components first class mangling candidates with their own tag. |
vdonaldson
approved these changes
Sep 13, 2023
It is possible for a derived type extending a type with private components to define components with the same name as the private components. This was not properly handled by lowering where several fir.record type component names could end-up being the same, leading to bad generated code (only the first component was accessed via fir.field_index, leading to bad generated code). This patch handles the situation by adding the derived type mangled name to private component.
jeanPerier
force-pushed
the
jpr-private-comp-name-clash-2
branch
from
d5c84bd
to
42a94b2
Compare
ZijunZhaoCCK
pushed a commit
to ZijunZhaoCCK/llvm-project
that referenced
this pull request
Sep 19, 2023
It is possible for a derived type extending a type with private components to define components with the same name as the private components. This was not properly handled by lowering where several fir.record type component names could end-up being the same, leading to bad generated code (only the first component was accessed via fir.field_index, leading to bad generated code). This patch handles the situation by adding the derived type mangled name to private component.
zahiraam
pushed a commit
to tahonermann/llvm-project
that referenced
this pull request
Oct 24, 2023
It is possible for a derived type extending a type with private components to define components with the same name as the private components. This was not properly handled by lowering where several fir.record type component names could end-up being the same, leading to bad generated code (only the first component was accessed via fir.field_index, leading to bad generated code). This patch handles the situation by adding the derived type mangled name to private component.
zahiraam
pushed a commit
to tahonermann/llvm-project
that referenced
this pull request
Oct 24, 2023
It is possible for a derived type extending a type with private components to define components with the same name as the private components. This was not properly handled by lowering where several fir.record type component names could end-up being the same, leading to bad generated code (only the first component was accessed via fir.field_index, leading to bad generated code). This patch handles the situation by adding the derived type mangled name to private component.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It is possible for a derived type extending a type with private components to define components with the same name as the private components.
This was not properly handled by lowering where several fir.record type component names could end-up being the same, leading to bad generated code (only the first component was accessed via fir.field_index, leading to bad generated code).
This patch handles the situation by adding the derived type mangled name to private component.