[PATCH] D87001: [IRMover] Avoid materializing global value that belongs to not-yet-linked module

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D87001#2317990 <https://reviews.llvm.org/D87001#2317990>, @ychen wrote:

> In D87001#2317777 <https://reviews.llvm.org/D87001#2317777>, @tejohnson wrote:
>
>> It sounds like even the test case that was added for D47898 <https://reviews.llvm.org/D47898> passes when that fix is reverted?
>
> Sorry, that's not what I meant. I meant to say that D47898 <https://reviews.llvm.org/D47898> regressed the test case in this patch. (Ps, I've verified D47898 <https://reviews.llvm.org/D47898> does fix its own test case).

Ok got it. I should have re-read the original summary again! You are essentially reverting D47898 <https://reviews.llvm.org/D47898> and replacing with a more comprehensive fix. Thanks for adding a test case. I added some suggestions to the test, mostly just comment clarity.



================
Comment at: llvm/test/LTO/X86/Inputs/type-mapping-bug4_1.ll:11
+; (step1) in stage1,
+; %class.std::unique_ptr_base.1(t1) is mapped to %class.std::unique_ptr_base(t0)
+; %class.CCSM(t1) is mapped to %class.CWBD(t0)
----------------
I assume t1 refers to %t1.o, etc? Would be clearer to spell that out here and elsewhere.


================
Comment at: llvm/test/LTO/X86/Inputs/type-mapping-bug4_1.ll:24
+define void @a() {
+  ; (without this fix)
+  ; (step3.1) mapping `%class.CB* undef` creates the first instance of %class.CB (%class.CB).
----------------
Since in the future it won't be obvious what "this" fix is, suggest making this something like "Without the fix in D87001 to delay materialization of `@d` until its module is linked".


================
Comment at: llvm/test/LTO/X86/type-mapping-bug4.ll:4
+; RUN: opt -module-summary -o %t2.o %s
+; RUN: llvm-lto2 run -o %ta %t0.o %t1.o %t2.o -r %t1.o,a,px -r %t2.o,d,px -r %t1.o,h,x -r %t2.o,h,x -r %t1.o,j,px
+
----------------
I assume this fails without this fix? Would also be good to add some checking of the resulting linked code.


================
Comment at: llvm/test/LTO/X86/type-mapping-bug4.ll:14
+
+; Remapping this type returns itself due to D47898 due to step3.1
+%class.CB = type { %"class.std::unique_ptr_base.2" }
----------------
The steps vs stages is confusing to me. Suggest noting above where you describe the stages where the steps fit. Also suggest mentioning where these steps are described, i.e. "due to step3.1 described in type-mapping-bug4_1.ll". Or maybe if you mention above where listing stages which step is included in which stage you could say there where each step is described.


================
Comment at: llvm/test/LTO/X86/type-mapping-bug4.ll:26
+define void @d(%class.CB*) {
+  ; (without the fix)
+  ; * SourceElementType %class.CB* is remapped to itself.
----------------
Similar comment to the one in type-mapping-bug_1.ll - explicitly specify what the fix is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87001