aaron.ballman added a comment. In D131858#4051352 <https://reviews.llvm.org/D131858#4051352>, @erichkeane wrote:
> In D131858#4051336 <https://reviews.llvm.org/D131858#4051336>, @aaron.ballman > wrote: > >> In D131858#4050112 <https://reviews.llvm.org/D131858#4050112>, @rsmith wrote: >> >>> In D131858#3957630 <https://reviews.llvm.org/D131858#3957630>, @arphaman >>> wrote: >>> >>>> This change has caused a failure in Clang's stage 2 CI on the green dragon >>>> Darwin CI: >>>> https://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/6390/console. >>>> >>>> Assertion failed: (lvaluePath->getType() == elemTy && "Unexpected type >>>> reference!"), function readAPValue, file >>>> /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/include/clang/AST/AbstractBasicReader.inc, >>>> line 736. >>> >>> This assert is simply wrong, and I've removed it in >>> rG2009f2450532450a99c1a03d5e2c30f478121839 >>> <https://reviews.llvm.org/rG2009f2450532450a99c1a03d5e2c30f478121839> -- >>> that change should be safe to cherry-pick into the release branch. It's >>> possible for the recomputation of the type after deserialization to result >>> in a different type than what we saw when serializing, because >>> redeclarations of the same entity can use the same type with different >>> sugar -- or even slightly different types in some cases, such as when an >>> array bound is added in a redeclaration. The dumps of the types provided by >>> @steven_wu confirms that we were just seeing a difference in type sugar in >>> this case. >>> >>>> Assertion failed: (BlockScope.empty() && CurAbbrevs.empty() && "Block >>>> imbalance"), function ~BitstreamWriter, file >>>> /Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/llvm/include/llvm/Bitstream/BitstreamWriter.h, >>>> line 119. >>> >>> Is this still happening? If so, this looks more serious, and will need >>> further investigation. >>> >>> Can we undo the workaround in https://reviews.llvm.org/D139956 and see if >>> the bot is now happy? Or can someone who was seeing problems before >>> (@steven_wu?) run a test? >> >> Thank you for poking at this Richard! However, I think we still need to >> revert the functionality in this area unless we're able to make headway on >> https://github.com/llvm/llvm-project/issues/59271 and quickly. FWIW, I ran >> into this exact problem yesterday on my dev machine, so the overhead is >> still a present concern. If that's something you plan to work on, then I >> think it'd make sense for Erich to hold off on starting the revert work to >> give you a chance to improve this. But if nobody is actively working on it, >> we need to start pulling this back because the branch date is a bit over a >> week away (Jan 24). > > My understanding is that the submitter of that bug did sufficient analysis to > determine that https://reviews.llvm.org/D136566 is the cause of his perf > regression, having done an analysis the patch before and after. The only > reason to revert THIS patch (and the follow-ups, since this is a 'base patch' > to the rest) is the report by @steven_wu . Ahhhh thank you for the extra information. I had missed that this was one before the perf issue. > SO, @steven_wu: Can ypu please, ASAP, try to reproduce your issue as Richard > asked above? IF so, we only have to revert D136566 > <https://reviews.llvm.org/D136566>, which should fix our performance issue. > (that is, revert the workaround you submitted in > https://reviews.llvm.org/D139956, then see if it works?). +1, but based on the link to the workaround and what Richard fixed, I'm optimistic we can keep this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131858/new/ https://reviews.llvm.org/D131858 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
