[PATCH] D16440: [ThinLTO] Link in only necessary DICompileUnit operands
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 28 13:34:53 PST 2016
tejohnson added a comment.
In http://reviews.llvm.org/D16440#338695, @dblaikie wrote:
> It's a little unclear from your description - are you importing /any/ types as definitions? Or are all imported types being imported as declarations? (I think the latter would be good/correct/minimal/simpler)
I think so. At least for composite types. Are there any cases, other than from the retainedTypes list on the compile unit, where a DICompositeType node is referenced by its address rather than by UUID? I haven't seen any, which means that we would not import any while mapping in metadata on imported functions/instructions. In that case they are only imported as declarations with this patch, since they are only imported by the new linkImportedCompileUnit routine.
Note that there are DIDerivedTypes that are referenced from imported functions (e.g. via the subprogram type array) and therefore mapped in when the functions and instructions are imported and mapped. Is there a distinction between definition and declaration for these types?
Comment at: test/Transforms/FunctionImport/funcimport_debug.ll:18
@@ +17,3 @@
+; Confirm that the enums didn't get pulled in to the imported compile unit
+; as it isn't needed here, but ensuring that retainedTypes immediately follows
> It might be clearer to match the retained types list (assuming all the types go in there, which I think they do - at least the ones using mangled names, etc) and then match all the types referenced from there, then you wouldn't need the CHECK-NOTs which are a bit brittle here anyway (if types are emitted in a different order, the CHECK-NOTs wouldn't catch them - they'd only catch the case where the type is emitted between the two surrounding CHECK'd types)
> Just a thought. (& could check that things like vtableHolder, etc, aren't emitted in the declarations you're interested in)
Good idea. I already found and fixed some issues where my existing CHECK-NOTs were not behaving as expected. Will do that.
More information about the llvm-commits