[PATCH] D88423: Fix llvm-link assert failure in BitCodeWriter

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/test/Linker/sret-types.ll:1
+; RUN: llvm-link %s %p/Inputs/sret-types-1.ll | llvm-dis | FileCheck %s
+
----------------
rsmith wrote:
> dblaikie wrote:
> > rsmith wrote:
> > > Simpler testcase (from https://reviews.llvm.org/D88241#2299465):
> > > 
> > > ```
> > > $ cat c.ll
> > > define void @f(i8* sret({i64})) { ret void }
> > > $ ./build/bin/opt - -o /tmp/tmp.ir # or anything that converts text IR to bitcode
> > > ```
> > > 
> > > There appear to be two issues here: the linker is not properly linking types inside attributes, and the value enumerator doesn't number types inside attributes. (I'm not sure if the linker relies on the value enumerator; maybe this patch fixes both issues, maybe not.)
> > If you have a chance, could you try this patch with both the test cases you posted over there? See if it does address both issues?
> This patch does *not* fix the failure of `llvm-link` to link together the modules properly. Indeed, this test fails if we:
> 
> ```
> -; RUN: llvm-link %s %p/Inputs/sret-types-1.ll | llvm-dis | FileCheck %s
> +; RUN: llvm-link %s %p/Inputs/sret-types-1.ll -S -o - | FileCheck %s
> ```
> 
> ... because it generates incorrect output that looks like:
> 
> ```
> ; ModuleID = 'llvm-link'
> source_filename = "llvm-link"
> 
> %v = type { i32 }
> 
> define void @g(%v* %0) {
>   ret void
> }
> 
> define void @f(%v* sret(%"type 0x2c18b30") %0) {
>   ret void
> }
> ```
> 
> So I guess there are actually *three* different bugs here: fixing the issue for the bitcode writer (as I can confirm this patch does) does not fix the corresponding issue for the textual IR printing.
Sounds like enough for me to be on board with reverting the original patch for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88423