erichkeane added a comment.

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 .

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?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to