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

[Sanne Wouda via Phabricator via llvm-commits]

sanwou01 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
+
----------------
tpopp wrote:
> 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.
Thanks, I was about to do the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88423