[flang][debug] Support derived types. #99476
Conversation
abidh
requested review from
tblah,
kiranchandramohan,
jeanPerier,
dpalermo,
jinisusan,
kiranktp and
vzakhari
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-codegen @llvm/pr-subscribers-flang-fir-hlfir Author: Abid Qadeer (abidh) ChangesThis PR adds initial debug support for derived type. It handles We use
The testcase checks for offsets of the components which had to be hardcoded in the test. So the testcase is currently enabled on x86_64. If a derived type has a component whose type is not supported by the With this PR in place, this is how the debugging of derived types look like: Patch is 22.02 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/99476.diff 6 Files Affected:
diff --git a/flang/include/flang/Optimizer/Dialect/FIRType.h b/flang/include/flang/Optimizer/Dialect/FIRType.h
index 3498a329ced30..bbb9cdeb9c1fe 100644
--- a/flang/include/flang/Optimizer/Dialect/FIRType.h
+++ b/flang/include/flang/Optimizer/Dialect/FIRType.h
@@ -487,10 +487,9 @@ std::string getTypeAsString(mlir::Type ty, const KindMapping &kindMap,
/// target dependent type size inquiries in lowering. It would also not be
/// straightforward given the need for a kind map that would need to be
/// converted in terms of mlir::DataLayoutEntryKey.
-std::pair<std::uint64_t, unsigned short>
-getTypeSizeAndAlignment(mlir::Location loc, mlir::Type ty,
- const mlir::DataLayout &dl,
- const fir::KindMapping &kindMap);
+std::pair<std::uint64_t, unsigned short> getTypeSizeAndAlignment(
+ mlir::Location loc, mlir::Type ty, const mlir::DataLayout &dl,
+ const fir::KindMapping &kindMap, bool *success = nullptr);
} // namespace fir
diff --git a/flang/lib/Optimizer/Dialect/FIRType.cpp b/flang/lib/Optimizer/Dialect/FIRType.cpp
index dbccacfa8be26..4d00ca058d506 100644
--- a/flang/lib/Optimizer/Dialect/FIRType.cpp
+++ b/flang/lib/Optimizer/Dialect/FIRType.cpp
@@ -1396,25 +1396,24 @@ void FIROpsDialect::registerTypes() {
std::pair<std::uint64_t, unsigned short>
fir::getTypeSizeAndAlignment(mlir::Location loc, mlir::Type ty,
const mlir::DataLayout &dl,
- const fir::KindMapping &kindMap) {
+ const fir::KindMapping &kindMap, bool *success) {
if (mlir::isa<mlir::IntegerType, mlir::FloatType, mlir::ComplexType>(ty)) {
llvm::TypeSize size = dl.getTypeSize(ty);
unsigned short alignment = dl.getTypeABIAlignment(ty);
return {size, alignment};
}
if (auto firCmplx = mlir::dyn_cast<fir::ComplexType>(ty)) {
- auto [floatSize, floatAlign] =
- getTypeSizeAndAlignment(loc, firCmplx.getEleType(kindMap), dl, kindMap);
+ auto [floatSize, floatAlign] = getTypeSizeAndAlignment(
+ loc, firCmplx.getEleType(kindMap), dl, kindMap, success);
return {llvm::alignTo(floatSize, floatAlign) + floatSize, floatAlign};
}
if (auto real = mlir::dyn_cast<fir::RealType>(ty))
- return getTypeSizeAndAlignment(loc, real.getFloatType(kindMap), dl,
- kindMap);
+ return getTypeSizeAndAlignment(loc, real.getFloatType(kindMap), dl, kindMap,
+ success);
if (auto seqTy = mlir::dyn_cast<fir::SequenceType>(ty)) {
auto [eleSize, eleAlign] =
- getTypeSizeAndAlignment(loc, seqTy.getEleTy(), dl, kindMap);
-
+ getTypeSizeAndAlignment(loc, seqTy.getEleTy(), dl, kindMap, success);
std::uint64_t size =
llvm::alignTo(eleSize, eleAlign) * seqTy.getConstantArraySize();
return {size, eleAlign};
@@ -1424,7 +1423,7 @@ fir::getTypeSizeAndAlignment(mlir::Location loc, mlir::Type ty,
unsigned short align = 1;
for (auto component : recTy.getTypeList()) {
auto [compSize, compAlign] =
- getTypeSizeAndAlignment(loc, component.second, dl, kindMap);
+ getTypeSizeAndAlignment(loc, component.second, dl, kindMap, success);
size =
llvm::alignTo(size, compAlign) + llvm::alignTo(compSize, compAlign);
align = std::max(align, compAlign);
@@ -1434,13 +1433,21 @@ fir::getTypeSizeAndAlignment(mlir::Location loc, mlir::Type ty,
if (auto logical = mlir::dyn_cast<fir::LogicalType>(ty)) {
mlir::Type intTy = mlir::IntegerType::get(
logical.getContext(), kindMap.getLogicalBitsize(logical.getFKind()));
- return getTypeSizeAndAlignment(loc, intTy, dl, kindMap);
+ return getTypeSizeAndAlignment(loc, intTy, dl, kindMap, success);
}
if (auto character = mlir::dyn_cast<fir::CharacterType>(ty)) {
mlir::Type intTy = mlir::IntegerType::get(
character.getContext(),
kindMap.getCharacterBitsize(character.getFKind()));
- return getTypeSizeAndAlignment(loc, intTy, dl, kindMap);
+ auto [compSize, compAlign] =
+ getTypeSizeAndAlignment(loc, intTy, dl, kindMap, success);
+ if (character.hasConstantLen())
+ compSize *= character.getLen();
+ return {compSize, compAlign};
+ }
+ if (success) {
+ *success = false;
+ return {0, 1};
}
TODO(loc, "computing size of a component");
}
diff --git a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
index 8bb24fb6c8078..6e6f38991f52c 100644
--- a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
+++ b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
@@ -68,13 +68,6 @@ class AddDebugInfoPass : public fir::impl::AddDebugInfoBase<AddDebugInfoPass> {
mlir::SymbolTable *symbolTable);
};
-static uint32_t getLineFromLoc(mlir::Location loc) {
- uint32_t line = 1;
- if (auto fileLoc = mlir::dyn_cast<mlir::FileLineColLoc>(loc))
- line = fileLoc.getLine();
- return line;
-}
-
bool debugInfoIsAlreadySet(mlir::Location loc) {
if (mlir::isa<mlir::FusedLoc>(loc))
return true;
@@ -159,13 +152,19 @@ void AddDebugInfoPass::handleGlobalOp(fir::GlobalOp globalOp,
return;
mlir::ModuleOp module = getOperation();
mlir::MLIRContext *context = &getContext();
- fir::DebugTypeGenerator typeGen(module);
+ fir::DebugTypeGenerator typeGen(module, symbolTable);
mlir::OpBuilder builder(context);
std::pair result = fir::NameUniquer::deconstruct(globalOp.getSymName());
if (result.first != fir::NameUniquer::NameKind::VARIABLE)
return;
+ // Discard entries that describe a derived type. Usually start with '.c.',
+ // '.dt.' or '.n.'. It would be better if result of the deconstruct had a flag
+ // for such values so that we dont have to look at string values.
+ if (!result.second.name.empty() && result.second.name[0] == '.')
+ return;
+
unsigned line = getLineFromLoc(globalOp.getLoc());
// DWARF5 says following about the fortran modules:
@@ -267,7 +266,7 @@ void AddDebugInfoPass::runOnOperation() {
mlir::StringAttr::get(context, result.second.name);
llvm::SmallVector<mlir::LLVM::DITypeAttr> types;
- fir::DebugTypeGenerator typeGen(module);
+ fir::DebugTypeGenerator typeGen(module, &symbolTable);
for (auto resTy : funcOp.getResultTypes()) {
auto tyAttr =
typeGen.convertType(resTy, fileAttr, cuAttr, funcOp.getLoc());
diff --git a/flang/lib/Optimizer/Transforms/DebugTypeGenerator.cpp b/flang/lib/Optimizer/Transforms/DebugTypeGenerator.cpp
index db559731552df..a94d1c41f56b0 100644
--- a/flang/lib/Optimizer/Transforms/DebugTypeGenerator.cpp
+++ b/flang/lib/Optimizer/Transforms/DebugTypeGenerator.cpp
@@ -16,6 +16,7 @@
#include "flang/Optimizer/CodeGen/DescriptorModel.h"
#include "flang/Optimizer/CodeGen/TypeConverter.h"
#include "flang/Optimizer/Support/DataLayout.h"
+#include "flang/Optimizer/Support/InternalNames.h"
#include "mlir/Pass/Pass.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/BinaryFormat/Dwarf.h"
@@ -44,8 +45,9 @@ std::uint64_t getComponentOffset<0>(const mlir::DataLayout &dl,
return 0;
}
-DebugTypeGenerator::DebugTypeGenerator(mlir::ModuleOp m)
- : module(m), kindMapping(getKindMapping(m)) {
+DebugTypeGenerator::DebugTypeGenerator(mlir::ModuleOp m,
+ mlir::SymbolTable *symbolTable_)
+ : module(m), symbolTable(symbolTable_), kindMapping(getKindMapping(m)) {
LLVM_DEBUG(llvm::dbgs() << "DITypeAttr generator\n");
std::optional<mlir::DataLayout> dl =
@@ -154,6 +156,58 @@ mlir::LLVM::DITypeAttr DebugTypeGenerator::convertBoxedSequenceType(
dataLocation, /*rank=*/nullptr, allocated, associated);
}
+mlir::LLVM::DITypeAttr DebugTypeGenerator::convertRecordType(
+ fir::RecordType Ty, mlir::LLVM::DIFileAttr fileAttr,
+ mlir::LLVM::DIScopeAttr scope, mlir::Location loc) {
+ mlir::MLIRContext *context = module.getContext();
+ auto result = fir::NameUniquer::deconstruct(Ty.getName());
+ if (result.first != fir::NameUniquer::NameKind::DERIVED_TYPE)
+ return genPlaceholderType(context);
+
+ std::optional<mlir::DataLayout> dl =
+ fir::support::getOrSetDataLayout(module, /*allowDefaultLayout=*/true);
+ if (!dl) {
+ mlir::emitError(module.getLoc(), "Missing data layout attribute in module");
+ return genPlaceholderType(context);
+ }
+ unsigned line = 1;
+ mlir::LLVM::DITypeAttr parentTypeAttr = nullptr;
+ fir::TypeInfoOp tiOp = symbolTable->lookup<fir::TypeInfoOp>(Ty.getName());
+ if (tiOp) {
+ line = getLineFromLoc(tiOp.getLoc());
+ if (fir::RecordType parentTy = tiOp.getIfParentType())
+ parentTypeAttr =
+ convertRecordType(parentTy, fileAttr, scope, tiOp.getLoc());
+ }
+
+ llvm::SmallVector<mlir::LLVM::DINodeAttr> elements;
+ std::uint64_t offset = 0;
+ for (auto [fieldName, fieldTy] : Ty.getTypeList()) {
+ bool success = true;
+ auto [byteSize, byteAlign] =
+ fir::getTypeSizeAndAlignment(loc, fieldTy, *dl, kindMapping, &success);
+ if (!success)
+ return genPlaceholderType(context);
+
+ mlir::LLVM::DITypeAttr elemTy = convertType(fieldTy, fileAttr, scope, loc);
+ offset = llvm::alignTo(offset, byteAlign);
+ mlir::LLVM::DIDerivedTypeAttr tyAttr = mlir::LLVM::DIDerivedTypeAttr::get(
+ context, llvm::dwarf::DW_TAG_member,
+ mlir::StringAttr::get(context, fieldName), elemTy, byteSize * 8,
+ byteAlign * 8, offset * 8, /*optional<address space>=*/std::nullopt,
+ /*extra data=*/nullptr);
+ elements.push_back(tyAttr);
+ offset += llvm::alignTo(byteSize, byteAlign);
+ }
+
+ return mlir::LLVM::DICompositeTypeAttr::get(
+ context, llvm::dwarf::DW_TAG_structure_type, /*recursive_id=*/{},
+ mlir::StringAttr::get(context, result.second.name), fileAttr, line, scope,
+ parentTypeAttr, mlir::LLVM::DIFlags::Zero, offset * 8,
+ /*alignInBits=*/0, elements, /*dataLocation=*/nullptr, /*rank=*/nullptr,
+ /*allocated=*/nullptr, /*associated=*/nullptr);
+}
+
mlir::LLVM::DITypeAttr DebugTypeGenerator::convertSequenceType(
fir::SequenceType seqTy, mlir::LLVM::DIFileAttr fileAttr,
mlir::LLVM::DIScopeAttr scope, mlir::Location loc) {
@@ -310,6 +364,8 @@ DebugTypeGenerator::convertType(mlir::Type Ty, mlir::LLVM::DIFileAttr fileAttr,
} else if (auto charTy = mlir::dyn_cast_or_null<fir::CharacterType>(Ty)) {
return convertCharacterType(charTy, fileAttr, scope, loc,
/*hasDescriptor=*/false);
+ } else if (auto recTy = mlir::dyn_cast_or_null<fir::RecordType>(Ty)) {
+ return convertRecordType(recTy, fileAttr, scope, loc);
} else if (auto boxTy = mlir::dyn_cast_or_null<fir::BoxType>(Ty)) {
auto elTy = boxTy.getElementType();
if (auto seqTy = mlir::dyn_cast_or_null<fir::SequenceType>(elTy))
diff --git a/flang/lib/Optimizer/Transforms/DebugTypeGenerator.h b/flang/lib/Optimizer/Transforms/DebugTypeGenerator.h
index ec881e8be7cad..12d215a9f5775 100644
--- a/flang/lib/Optimizer/Transforms/DebugTypeGenerator.h
+++ b/flang/lib/Optimizer/Transforms/DebugTypeGenerator.h
@@ -23,7 +23,7 @@ namespace fir {
/// This converts FIR/mlir type to DITypeAttr.
class DebugTypeGenerator {
public:
- DebugTypeGenerator(mlir::ModuleOp module);
+ DebugTypeGenerator(mlir::ModuleOp module, mlir::SymbolTable *symbolTable);
mlir::LLVM::DITypeAttr convertType(mlir::Type Ty,
mlir::LLVM::DIFileAttr fileAttr,
@@ -31,6 +31,10 @@ class DebugTypeGenerator {
mlir::Location loc);
private:
+ mlir::LLVM::DITypeAttr convertRecordType(fir::RecordType Ty,
+ mlir::LLVM::DIFileAttr fileAttr,
+ mlir::LLVM::DIScopeAttr scope,
+ mlir::Location loc);
mlir::LLVM::DITypeAttr convertSequenceType(fir::SequenceType seqTy,
mlir::LLVM::DIFileAttr fileAttr,
mlir::LLVM::DIScopeAttr scope,
@@ -57,6 +61,7 @@ class DebugTypeGenerator {
bool genAllocated, bool genAssociated);
mlir::ModuleOp module;
+ mlir::SymbolTable *symbolTable;
KindMapping kindMapping;
std::uint64_t dimsSize;
std::uint64_t dimsOffset;
@@ -66,4 +71,11 @@ class DebugTypeGenerator {
} // namespace fir
+static uint32_t getLineFromLoc(mlir::Location loc) {
+ uint32_t line = 1;
+ if (auto fileLoc = mlir::dyn_cast<mlir::FileLineColLoc>(loc))
+ line = fileLoc.getLine();
+ return line;
+}
+
#endif // FORTRAN_OPTIMIZER_TRANSFORMS_DEBUGTYPEGENERATOR_H
diff --git a/flang/test/Transforms/debug-derived-type-1.fir b/flang/test/Transforms/debug-derived-type-1.fir
new file mode 100644
index 0000000000000..700e2fb09b291
--- /dev/null
+++ b/flang/test/Transforms/debug-derived-type-1.fir
@@ -0,0 +1,73 @@
+// RUN: fir-opt --add-debug-info --mlir-print-debuginfo %s | FileCheck %s
+
+// Only enabled on x86_64
+// REQUIRES: x86-registered-target
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<!llvm.ptr<272>, dense<64> : vector<4xi64>>, #dlti.dl_entry<!llvm.ptr<271>, dense<32> : vector<4xi64>>, #dlti.dl_entry<!llvm.ptr<270>, dense<32> : vector<4xi64>>, #dlti.dl_entry<f64, dense<64> : vector<2xi64>>, #dlti.dl_entry<f80, dense<128> : vector<2xi64>>, #dlti.dl_entry<f128, dense<128> : vector<2xi64>>, #dlti.dl_entry<i128, dense<128> : vector<2xi64>>, #dlti.dl_entry<i64, dense<64> : vector<2xi64>>, #dlti.dl_entry<i16, dense<16> : vector<2xi64>>, #dlti.dl_entry<i32, dense<32> : vector<2xi64>>, #dlti.dl_entry<f16, dense<16> : vector<2xi64>>, #dlti.dl_entry<!llvm.ptr, dense<64> : vector<4xi64>>, #dlti.dl_entry<i1, dense<8> : vector<2xi64>>, #dlti.dl_entry<i8, dense<8> : vector<2xi64>>, #dlti.dl_entry<"dlti.stack_alignment", 128 : i64>, #dlti.dl_entry<"dlti.endianness", "little">>, fir.defaultkind = "a1c4d8i4l4r4", fir.kindmap = "", llvm.data_layout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128", llvm.target_triple = "x86_64-unknown-linux-gnu", omp.is_gpu = false, omp.is_target_device = false, omp.version = #omp.version<version = 11>} {
+ fir.global @_QMm_employeeEemployee : !fir.type<_QMm_employeeTt_employee{t_person:!fir.type<_QMm_employeeTt_person{t_address:!fir.type<_QMm_employeeTt_address{house_number:i32}>,name:!fir.char<1,20>}>,hired_date:!fir.type<_QMm_employeeTt_date{year:i32,month:i32,day:i32}>,monthly_salary:f32}> {
+ %0 = fir.zero_bits !fir.type<_QMm_employeeTt_employee{t_person:!fir.type<_QMm_employeeTt_person{t_address:!fir.type<_QMm_employeeTt_address{house_number:i32}>,name:!fir.char<1,20>}>,hired_date:!fir.type<_QMm_employeeTt_date{year:i32,month:i32,day:i32}>,monthly_salary:f32}>
+ fir.has_value %0 : !fir.type<_QMm_employeeTt_employee{t_person:!fir.type<_QMm_employeeTt_person{t_address:!fir.type<_QMm_employeeTt_address{house_number:i32}>,name:!fir.char<1,20>}>,hired_date:!fir.type<_QMm_employeeTt_date{year:i32,month:i32,day:i32}>,monthly_salary:f32}>
+ } loc(#loc5)
+ fir.global @_QMt1Evar : !fir.type<_QMt1Tt_t1{age:i32,points:!fir.array<3x!fir.complex<4>>,cond:!fir.logical<1>,name:!fir.char<1,20>,ratio:f64}> {
+ %0 = fir.zero_bits !fir.type<_QMt1Tt_t1{age:i32,points:!fir.array<3x!fir.complex<4>>,cond:!fir.logical<1>,name:!fir.char<1,20>,ratio:f64}>
+ fir.has_value %0 : !fir.type<_QMt1Tt_t1{age:i32,points:!fir.array<3x!fir.complex<4>>,cond:!fir.logical<1>,name:!fir.char<1,20>,ratio:f64}>
+ } loc(#loc6)
+ fir.type_info @_QMt1Tt_t1 noinit nodestroy nofinal : !fir.type<_QMt1Tt_t1{age:i32,points:!fir.array<3x!fir.complex<4>>,cond:!fir.logical<1>,name:!fir.char<1,20>,ratio:f64}> loc(#loc7)
+ fir.type_info @_QMm_employeeTt_address noinit nodestroy nofinal : !fir.type<_QMm_employeeTt_address{house_number:i32}> loc(#loc1)
+ fir.type_info @_QMm_employeeTt_person noinit nodestroy nofinal extends !fir.type<_QMm_employeeTt_address{house_number:i32}> : !fir.type<_QMm_employeeTt_person{t_address:!fir.type<_QMm_employeeTt_address{house_number:i32}>,name:!fir.char<1,20>}> loc(#loc2)
+ fir.type_info @_QMm_employeeTt_date noinit nodestroy nofinal : !fir.type<_QMm_employeeTt_date{year:i32,month:i32,day:i32}> loc(#loc3)
+ fir.type_info @_QMm_employeeTt_employee noinit nodestroy nofinal extends !fir.type<_QMm_employeeTt_person{t_address:!fir.type<_QMm_employeeTt_address{house_number:i32}>,name:!fir.char<1,20>}> : !fir.type<_QMm_employeeTt_employee{t_person:!fir.type<_QMm_employeeTt_person{t_address:!fir.type<_QMm_employeeTt_address{house_number:i32}>,name:!fir.char<1,20>}>,hired_date:!fir.type<_QMm_employeeTt_date{year:i32,month:i32,day:i32}>,monthly_salary:f32}> loc(#loc4)
+ fir.type_info @_QFTt_pair noinit nodestroy nofinal : !fir.type<_QFTt_pair{i:i64,x:f64}> loc(#loc8)
+ func.func @_QQmain() attributes {fir.bindc_name = "test"} {
+ %1 = fir.alloca !fir.type<_QFTt_pair{i:i64,x:f64}> {bindc_name = "pair", uniq_name = "_QFEpair"}
+ %2 = fircg.ext_declare %1 {uniq_name = "_QFEpair"} : (!fir.ref<!fir.type<_QFTt_pair{i:i64,x:f64}>>) -> !fir.ref<!fir.type<_QFTt_pair{i:i64,x:f64}>> loc(#loc9)
+ return
+ } loc(#loc10)
+}
+#loc1 = loc("derived1.f90":24:1)
+#loc2 = loc("derived1.f90":35:25)
+#loc3 = loc("derived1.f90":17:1)
+#loc4 = loc("derived1.f90":46:1)
+#loc5 = loc("derived1.f90":50:3)
+#loc6 = loc("derived1.f90":62:3)
+#loc7 = loc("derived1.f90":70:3)
+#loc8 = loc("derived1.f90":85:3)
+#loc9 = loc("derived1.f90":77:3)
+#loc10 = loc("derived1.f90":75:3)
+
+
+// CHECK-DAG: #[[INT_TY:.*]] = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "integer", sizeInBits = 32, encoding = DW_ATE_signed>
+// CHECK-DAG: #[[INT8_TY:.*]] = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "integer", sizeInBits = 64, encoding = DW_ATE_signed>
+// CHECK-DAG: #[[REAL4_TY:.*]] = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "real", sizeInBits = 32, encoding = DW_ATE_float>
+// CHECK-DAG: #[[CMX8_TY:.*]] = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "complex", sizeInBits = 64, encoding = DW_ATE_complex_float>
+// CHECK-DAG: #[[CMX_ARR:.*]] = #llvm.di_composite_type<tag = DW_TAG_array_type, baseType = #[[CMX8_TY:.*]]{{.*}}>
+// CHECK-DAG: #[[LOG_TY:.*]] = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "logical", sizeInBits = 8, encoding = DW_ATE_boolean>
+// CHECK-DAG: #[[REAL8_TY:.*]] = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "real", sizeInBits = 64, encoding = DW_ATE_float>
+// CHECK-DAG: #[[STR_TY:.*]] = #llvm.di_string_type
+// CHECK-DAG: #[[MOD:.*]] = #llvm.di_module<{{.*}}name = "m_employee"{{.*}}>
+// CHECK-DAG: #[[MOD1:.*]] = #llvm.di_module<{{.*}}name = "t1"{{.*}}>
+// CHECK-DAG: #[[ELMA1:.*]] = #llvm.di_derived_type<tag = DW_TAG_member, name = "house_number", baseType = #[[INT_TY]], sizeInBits = 32, alignInBits = 32>
+// CHECK-DAG: #[[ADDR:.*]] = #llvm.di_composite_type<tag = DW_TAG_structure_type, name = "t_address"{{.*}}line = 24, scope = #[[MOD]], sizeInBits = 32, elements = #[[ELMA1]]>
+// CHECK-DAG: #[[ELMD1:.*]] = #llvm.di_derived_type<tag = DW_TAG_member, name = "year", baseType = #[[INT_TY]], sizeInBits = 32, alignInBits = 32>
+// CHECK-DAG: #[[ELMD2:.*]] = #llvm.di_derived_type<tag = DW_TAG_member, name = "month", baseType = #[[INT_TY]], sizeInBits = 32, alignInBits = 32, offsetInBits = 32>
+// CHECK-DAG: #[[ELMD3:.*]] = #llvm.di_derived_type<tag = DW_TAG_member, name = "day", baseType = #[[INT_TY]], sizeInBits = 32, alignInBits = 32, offsetInBits = 64>
+// CHECK-DAG: #[[DATE:.*]] = #llvm.di_composite_type<tag = DW_TAG_structure_type, name = "t_date", file = #di_file, line = 17, scope = #[[MOD]], sizeInBits = 96, elements = #[[ELMD1]], #[[ELMD2]], #[[ELMD3]]>
+// CHECK-DAG: #[[ELMP1:.*]] = #llvm.di_derived_type<tag = DW_TAG_member, name = "t_address", baseType = #[[ADDR]], sizeInBits = 32, alignInBits = 32>
+// CHECK-DAG: #[[ELMP2:.*]] = #llvm.di_derived_type<tag = DW_TAG_member, name = "name", baseType = #[[STR_TY]], sizeInBits = 160, alignInBits = 8, offsetInBits = 32>
+// CHECK-DAG: #[[PERS:.*]] = #llvm.di_composite_type<tag = DW_TAG_structure_type, name = "t_person"{{.*}}line = 35, scope = #[[MOD]], baseType = #[[ADDR]], sizeInBits = 192, elements = #[[ELMP1]], #[[ELMP2]]>
+// CHECK-DAG: #[[ELME1:.*]] = #llvm.di_derived_type<tag = DW_TAG_member, name = "t_person", baseType = #[[PERS]], sizeInBits = 192, alignInBits = 32>
+// CHECK-DAG: #[[ELME2:.*]] = #llvm.di_derived_type<tag = DW_TAG_me...
[truncated]
|
There was a problem hiding this comment.
Style-wise, I would prefer something like
std::optional<std::pair<uint64_t, unsigned short>> getTypeSizeAndAlignment(...)
So that callers cannot forget to check for the error condition and we do not return anything if nothing is known.
It would be a pain to add the TODO message to every callsite. Maybe putting it in a wrapper called something like getSizeAndAlignmentOrCrash which unwraps the optional and prints the TODO message could be one approach.
I think this would be cleaner, but I am open to other ideas.
As for the rest, the code looks good, but please wait for review by somebody more familiar with derived types.
| const fir::KindMapping &kindMap); | ||
| std::pair<std::uint64_t, unsigned short> getTypeSizeAndAlignment( | ||
| mlir::Location loc, mlir::Type ty, const mlir::DataLayout &dl, | ||
| const fir::KindMapping &kindMap, bool *success = nullptr); |
There was a problem hiding this comment.
nit: if we are taking this approach, please could you add something to the documentation comment about the success parameter because I think the new behavior is not obvious
| auto [byteSize, byteAlign] = | ||
| fir::getTypeSizeAndAlignment(loc, fieldTy, *dl, kindMapping, &success); | ||
| if (!success) | ||
| return genPlaceholderType(context); |
There was a problem hiding this comment.
fir::getTypeSizeAndAlignment() does not seem to deal with pointer types (as you have mentioned, we still have unsupported types with which we are dealing in an incremental manner). Hence if the derived type has a pointer member in it (as in the example below), a DICompositeType does not get created for that derived type. Not sure if it would make better sense to create a DICompositeType with whatever members we have discovered so far instead of bailing out. This would provide better debuggability than no DIE getting created eventually for the derived type at all.
type dtype
integer :: i
real :: fi
character :: ci
real, pointer :: justaptr
end type dtype
There was a problem hiding this comment.
Thanks for the review. The issue I see is that we need the size of the components earlier in the derived type to get the offset of components that come after them. If we don't know the size of any member, it will result in wrong offset of all the components coming after that and debugger will show wrong values for them.
There was a problem hiding this comment.
Ah, I understand your point now. I have updated the PR as per your suggestion.
I have now changed the function to return an |
There was a problem hiding this comment.
Thanks for the update. Please wait for approval from @jinisusan
| std::optional<mlir::DataLayout> dl = | ||
| fir::support::getOrSetDataLayout(module, /*allowDefaultLayout=*/true); |
There was a problem hiding this comment.
Building a datalayout is not cheap and leverage caching to speed things up (contains many DenseMap, requires doing parsing of module attributes): it should be done only once at the pass level.
llvm-project/mlir/docs/DataLayout.md
Line 84 in af1d2b9
There was a problem hiding this comment.
Thanks for the suggestion. I have now updated the code to initialize DataLayout once and use it wherever needed.
| parentTypeAttr = | ||
| convertRecordType(parentTy, fileAttr, scope, tiOp.getLoc()); |
There was a problem hiding this comment.
There will also be an element created for the first component which is the parent component. Does dwarf expect that, or should the parent not be generated as a component?
There was a problem hiding this comment.
Yes the base as first argument works. The DWARF output is same as generated by classic flang and gfortran. The testcase I added has 3 level of parent-child relationship.
While looking at this, I noticed that I was also setting base_type field of the DICompositeTypeAttr in this case which is really not needed. That field is for situation where DICompositeTypeAttr is representing an array. I will fix it.
| @@ -154,6 +156,56 @@ mlir::LLVM::DITypeAttr DebugTypeGenerator::convertBoxedSequenceType( | |||
| dataLocation, /*rank=*/nullptr, allocated, associated); | |||
| } | |||
|
|
|||
| mlir::LLVM::DITypeAttr DebugTypeGenerator::convertRecordType( | |||
There was a problem hiding this comment.
This processing is likely not cheap, do you know how many times this will being called for a given derived type in a program: once per type, or once for every variable with this type, or more?
It may be worth having some caching fir::RecordType Ty <-> mlir::LLVM::DITypeAttr, or even mlir::Type <-> mlir::LLVM::DITypeAttr
There was a problem hiding this comment.
Keeping a cache for mlir::Type to mlir::DITypeAttr was always the plan. Although I have noticed in the IR dump that there is no type duplication. So may be mlir does caching at some level. I will keep this thing as a TODO to avoid generating redundant types.
| if (!result) | ||
| break; | ||
| auto [byteSize, byteAlign] = *result; | ||
| mlir::LLVM::DITypeAttr elemTy = convertType(fieldTy, fileAttr, scope, loc); |
There was a problem hiding this comment.
Beware that derived type may be recursive, directly or indirectly with pointer/allocatable components.
You may have to do something special here to prevent infinite loops.
module m
type t1
type(t2), pointer :: p
end type
type t2
type(t1) :: t1
end type
type(t2) :: some_global
end module
My guess is that for now you may be OK because getTypeSizeAndAlignment probably returns nullopt for pointer/allocatable component since it does not know how to compute descriptor size, but you may want to add a note and a test to check that this is not causing compiler crashes with -g.
There was a problem hiding this comment.
I have added a test for now to make sure we don't crash for such cases. I will handle it when I add support for allocatable components. This is is common issue in C like language so there are ways to handle it.
|
Hi @jeanPerier |
|
Hi @jeanPerier |
This PR adds initial debug support for derived type. It handles `RecordType` and generates appropriate `DICompositeTypeAttr`. The `TypeInfoOp` is used to get information about the parent and location of the derived type. We use `getTypeSizeAndAlignment` to get the size and alignment of the components of the derived types. This function needed a few changes to be suitable to be used here: 1. The `getTypeSizeAndAlignment` errored out on unsupported type which would not work with incremental way we are building debug support. I have added a new parameter. If it is null, the function behaves same as before and calls `TODO()`. If this is not null, then a true/false status is returned which can be used by clients to see if function succeeded. This is not very clean but gets the job done without duplicating any code. 2. The Character type was returning size of just element and not the whole string which has been fixed.
Renamed `getTypeSizeAndAlignment` to `getTypeSizeAndAlignmentOrCrash`. It makes it clear to user that function terminates the compilation if it can't handle the given type. The `getTypeSizeAndAlignment` now returns a pair wrapped into an std::optional which can be used to detect error.
If we encounter a type whose size can't be determined, we generate a derived type with whatever members we have instead of generating a place holder type.
1. Initialize DataLayout only once in the pass. 2. Remove baseType field from the DICompositeTypeAttr. 2. Add a test to make sure we don't crash with recursive types.
|
Hi @jeanPerier |
This PR adds initial debug support for derived type. It handles
`RecordType` and generates appropriate `DICompositeTypeAttr`. The
`TypeInfoOp` is used to get information about the parent and location of
the derived type.
We use `getTypeSizeAndAlignment` to get the size and alignment of the
components of the derived types. This function needed a few changes to
be suitable to be used here:
1. The `getTypeSizeAndAlignment` errored out on unsupported type which
would not work with incremental way we are building debug support. A new
variant of this function has been that returns an std::optional. The original
function has been renamed to `getTypeSizeAndAlignmentOrCrash` as it
will call `TODO()` for unsupported types.
2. The Character type was returning size of just element and not the
whole string which has been fixed.
The testcase checks for offsets of the components which had to be
hardcoded in the test. So the testcase is currently enabled on x86_64.
With this PR in place, this is how the debugging of derived types look
like:
```
type :: t_date
integer :: year, month, day
end type
type :: t_address
integer :: house_number
end type
type, extends(t_address) :: t_person
character(len=20) name
end type
type, extends(t_person) :: t_employee
type(t_date) :: hired_date
real :: monthly_salary
end type
type(t_employee) :: employee
(gdb) p employee
$1 = ( t_person = ( t_address = ( house_number = 1 ), name = 'John', ' ' <repeats 16 times> ), hired_date = ( year = 2020, month = 1, day = 20 ), monthly_salary = 3.1400001 )
```
This PR adds initial debug support for derived type. It handles
`RecordType` and generates appropriate `DICompositeTypeAttr`. The
`TypeInfoOp` is used to get information about the parent and location of
the derived type.
We use `getTypeSizeAndAlignment` to get the size and alignment of the
components of the derived types. This function needed a few changes to
be suitable to be used here:
1. The `getTypeSizeAndAlignment` errored out on unsupported type which
would not work with incremental way we are building debug support. A new
variant of this function has been that returns an std::optional. The original
function has been renamed to `getTypeSizeAndAlignmentOrCrash` as it
will call `TODO()` for unsupported types.
2. The Character type was returning size of just element and not the
whole string which has been fixed.
The testcase checks for offsets of the components which had to be
hardcoded in the test. So the testcase is currently enabled on x86_64.
With this PR in place, this is how the debugging of derived types look
like:
```
type :: t_date
integer :: year, month, day
end type
type :: t_address
integer :: house_number
end type
type, extends(t_address) :: t_person
character(len=20) name
end type
type, extends(t_person) :: t_employee
type(t_date) :: hired_date
real :: monthly_salary
end type
type(t_employee) :: employee
(gdb) p employee
$1 = ( t_person = ( t_address = ( house_number = 1 ), name = 'John', ' ' <repeats 16 times> ), hired_date = ( year = 2020, month = 1, day = 20 ), monthly_salary = 3.1400001 )
```
This PR adds initial debug support for derived type. It handles
RecordTypeand generates appropriateDICompositeTypeAttr. TheTypeInfoOpis used to get information about the parent and location of the derived type.We use
getTypeSizeAndAlignmentto get the size and alignment of the components of the derived types. This function needed a few changes to be suitable to be used here:The
getTypeSizeAndAlignmenterrored out on unsupported type which would not work with incremental way we are building debug support. I have added a newsuccessparameter. If it is null, the function behaves same as before and callsTODO()for unsupported type. If this is not null, then a true/false status is returned which can be used by clients to see if function succeeded. This is not very clean but gets the job done without duplicating any code.The Character type was returning size of just element and not the whole string which has been fixed.
The testcase checks for offsets of the components which had to be hardcoded in the test. So the testcase is currently enabled on x86_64.
If a derived type has a component whose type is not supported by the
getTypeSizeAndAlignmentthen we will generate a place holder type for such derived type.With this PR in place, this is how the debugging of derived types look like: