[PATCH] D127625: [Debug] [Coroutines] Get rid of DW_ATE_address

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu marked an inline comment as done.
ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:925
     RetType = Builder.createBasicType(OS.str(), Layout.getTypeSizeInBits(Ty),
-                                      dwarf::DW_ATE_address,
-                                      llvm::DINode::FlagArtificial);
+      dwarf::DW_ATE_unsigned_char, llvm::DINode::FlagArtificial);
   }
----------------
dblaikie wrote:
> How come this one became `unsigned char` rather than pointer type?
The intention of the 'else' section is to create a DIType for an unresolved type. And the unresolved type lives in the coroutine frame. So it is a little bit odd for me to treat it as a pointer type. Since the size of the unresolved type might be larger than a pointer. 

The choice to use `DW_ATE_unsigned_char` might not be correct. Here is my thoughts. I want to treat the unresolved type as bytes. I want to make it as something like "I don't know what you are so I will present the bytes directly". And `DW_ATE_unsigned_char` is the closest thing I found in DW_ATE* things. How do you think about it?

This is covered in the test as vector types. Since we failed to recognize vector types now, we would treat it as a unresolved type with 128 size.


================
Comment at: llvm/test/Transforms/Coroutines/coro-debug-coro-frame.ll:36
+; CHECK-DAG: ![[INT64_PTR]] = !DIDerivedType(tag: DW_TAG_member, name: "__int_64_Ptr_3",{{.*}} baseType: ![[INT64_PTR_BASE:[0-9]+]]
+; CHECK-DAG: ![[INT64_PTR_BASE]] = !DIDerivedType(tag: DW_TAG_pointer_type, name: "__int_64_Ptr", baseType: null, size: 64
+; CHECK-DAG: ![[INT32_2]] = !DIDerivedType(tag: DW_TAG_member, name: "__int_32_4", scope: ![[FRAME_TYPE]], file: ![[FILE]], line: [[PROMISE_VAR_LINE]], baseType: ![[I32_BASE:[0-9]+]]{{.*}}, flags: DIFlagArtificial
----------------
dblaikie wrote:
> Why does this type have a name? I think it should probably be nameless - so it should merge with other instances of the same pointer type created for the original source code, etc.
The name here is try to provide more information as much as possible. Ideally, I think this one should be the `int*` type. It shows the the ability of type resolver in current coroutine compiler is not strong enough. Given this revision aims to get rid of DW_ATE_address, I would try to fix the problem in other revisions.

For the duplicated DITypes problem, this is tested in the following function 'bar' that the generated DIType would be reused.


================
Comment at: llvm/test/Transforms/Coroutines/coro-debug-coro-frame.ll:63-64
 declare void @consume(%struct.big_structure*)
+declare void @produce_vector(<4 x i32> *)
+declare void @consume_vector(<4 x i32> *)
 declare void @pi32(i32*)
----------------
dblaikie wrote:
> why are there changes to the IR here? I guess they're necessary to create the interesting debug info IR being tested?
This is intended to test the result of unresolved type.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127625/new/

https://reviews.llvm.org/D127625