[PATCH] D53267: [CodeExtractor] Erase debug intrinsics in outlined thunks

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

In https://reviews.llvm.org/D53267#1265690, @tejohnson wrote:

> In https://reviews.llvm.org/D53267#1265681, @vsk wrote:
>
> > In https://reviews.llvm.org/D53267#1265680, @tejohnson wrote:
> >
> > > In https://reviews.llvm.org/D53267#1265670, @tejohnson wrote:
> > >
> > > > In https://reviews.llvm.org/D53267#1265669, @vsk wrote:
> > > >
> > > > > In https://reviews.llvm.org/D53267#1265641, @sebpop wrote:
> > > > >
> > > > > > Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900 
> > > > > >  In which case you may want to add the testcases from that bug.
> > > > >
> > > > >
> > > > > It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.
> > > > >
> > > > > Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run `-Os -g -mllvm -hot-cold-split={true, false}` and check that the two CRCs were the same.
> > > >
> > > >
> > > > Thanks for the fix. I didn't see this patch before I updated that bug. This should fix my issue, which related to some llvm.dbg.value intrinsics in outlined code that weren't updated properly.
> > >
> > >
> > > I should add that in the one case I looked at, the first argument to the outlined llvm.dbg.value was not updated correctly, which seems a little different than the failure mode here. So there might be multiple issues with these outlined intrinsics that need fixing. Not sure if you want to update the comment.
> >
> >
> > Hm, I think that's the same failure mode I saw. A dbg.value from the original function was moved to the outlined function, but its ValueAsMetadata operand (the "function-local metadata") wasn't updated to point to an Argument in the outlined function.
>
>
> Yeah, sounds like it.
>
> > The error I'm referring to comes from Verifier::visitValueAsMetadata.
>
> Yep, I saw that one when building with hot cold splitting and -g. Originally the failure I saw was different, because it manifests differently when building with ThinLTO, where essentially the bitcode gets corrupted because the operand types don't match.


I think I know what you're talking about. I saw something similar while building an internal framework with -flto=thin: with the verifier disabled, it manifested as an assertion failure in the bitcode writer's ValueEnumerator. But the underlying issue is the same as the one diagnosed by the verifier.


https://reviews.llvm.org/D53267