[PATCH] D31963: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: test/Transforms/ThinLTOBitcodeWriter/comdat.ll:5-26
+; MERGED: ${{"?lwt[^ ]+}} = comdat any
+; MERGED: $nlwt = comdat any
+; MERGED: {{@"?lwt_aliasee[^ ]*}} = private unnamed_addr global
+; MERGED-SAME: comdat(${{"?lwt[^ ]+}})
+; MERGED: {{@"?lwt_nl[^ ]+}} = hidden unnamed_addr global
+; MERGED-SAME: comdat(${{"?lwt[^ ]+}})
+; MERGED: {{@"?nlwt_aliasee[^ ]*}} = private unnamed_addr global
----------------
inglorion wrote:
> pcc wrote:
> > Please move these checks next to the corresponding input IR (where possible).
> What is a good way to do that? When I wrote the checks next to the IR, I got errors, because the IR was emitted in a different order from how I wrote it. Specifically, all the comdat declarations come first, then all the globals, then all the functions. So I could reorder my code to be in that same order, but that would mix things from different comdats together, which, as you pointed out elsewhere, made the test harder to read. I thought this was a reasonable compromise - putting all the same comdats together in the IR, and putting the same modules together in the checks. But if you have a better suggestion, I'm all ears.
The main thing that made your previous test hard to read was the fact that the global variables from different comdats were not grouped, combined with the choice of names. It's fine if you separate comdats from global variables from aliases from functions, as long as the global variables are grouped and all names are prefixed.


================
Comment at: test/Transforms/ThinLTOBitcodeWriter/comdat.ll:41
+
+define i8* @lwt_fun() {
+  %1 = load i32, i32* @lwt_nl
----------------
I think you need to add a `comdat($lwt)` here if you intend this to be a member of `lwt`. It also doesn't look like you have any tests for this function.


================
Comment at: test/Transforms/ThinLTOBitcodeWriter/comdat.ll:59
+
+define i8* @nlwt_fun() {
+  %1 = load i32, i32* @nlwt
----------------
Same here.


================
Comment at: test/Transforms/ThinLTOBitcodeWriter/comdat.ll:74
+
+define i8* @nt_fun() {
+  %1 = load i32, i32* @nt_nl
----------------
Same here.


https://reviews.llvm.org/D31963