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

[Theodore Popp via Phabricator via llvm-commits]

tpopp 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
+
----------------
dblaikie wrote:
> 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.
I have gone ahead and reverted the original patch for now. I apologize for any difficulties this causes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88423