FWIW this is on my list. A quick glance sounded reasonable, I've not looked at it in detail though.<br><br>Thanks :)<div><br></div><div>-eric</div><br><div class="gmail_quote">On Mon Jan 12 2015 at 12:51:09 PM Ahmed Bougacha <<a href="mailto:ahmed.bougacha@gmail.com">ahmed.bougacha@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Ping for opinions!<br>
<br>
Thanks,<br>
- Ahmed<br>
<br>
<br>
On Wed, Jan 7, 2015 at 4:06 AM, Ahmed Bougacha <<a href="mailto:ahmed.bougacha@gmail.com" target="_blank">ahmed.bougacha@gmail.com</a>> wrote:<br>
> W.r.t. trunk not crashing on the testcase: I bisected it down to<br>
> r221973 making the assertion not fire in some cases where it used to.<br>
> I went ahead and changed the assertion in r225338.  Now the attached<br>
> testcase crashes, as it should.<br>
><br>
> - Ahmed<br>
><br>
> On Tue, Jan 6, 2015 at 2:49 PM, Ahmed Bougacha <<a href="mailto:ahmed.bougacha@gmail.com" target="_blank">ahmed.bougacha@gmail.com</a>> wrote:<br>
>> Hi all,<br>
>><br>
>> In some situations (see below for testcase explanation), we were<br>
>> crashing in various places, because of a single root problem: a<br>
>> MachineBasicBlock with DBG_VALUEs between the two terminators (a<br>
>> conditional and an unconditional branch).<br>
>><br>
>> It all goes downhill from there, with at least 4 different places<br>
>> where this breaks code assuming that the terminator before-last is<br>
>> immediately preceding the last one.<br>
>><br>
>> Now, if the DBG_VALUEs weren't there in the first place, we wouldn't<br>
>> have this problem.  However, most code already accepts DBG_VALUEs at<br>
>> the end of a block, after the last terminator.  My understanding is,<br>
>> it's not ideal nor well-tested, but we can have them between<br>
>> terminators as well.  If not, we should make it explicit.<br>
>><br>
>> Attached are 5 small patches, the most important being:<br>
>><br>
>>     [CodeGen] Add a MBB::getPreviousTerminator method, ignoring DBG_VALUEs.<br>
>>       ...<br>
>>     This commit adds a similar method, that returns an iterator to the<br>
>>     last terminator preceding the instruction passed as an iterator.<br>
>><br>
>> The others fix various uses in EarlyIfCvt, MBB, and AArch64, all<br>
>> breaking on my local testcase:<br>
>><br>
>>     [CodeGen] Ignore DBG_VALUEs between terminators when replacing MBB uses.<br>
>>     [CodeGen][EarlyIfCvt] Ignore DBG_VALUEs between terminators.<br>
>>     [AArch64] Ignore DBG_VALUEs in AnalyzeBranch, by using<br>
>> getPreviousTerminator.<br>
>>     [AArch64] Ignore DBG_VALUEs in RemoveBranch, by using getPreviousTerminator.<br>
>><br>
>> These fix the testcase, but I counted at least 5 other suspicious uses<br>
>> of isTerminator.<br>
>> Also, there are a few innocuous loops that can be replaced by<br>
>> getPreviousTerminator.<br>
>> If the general idea is sound, I'll fix those as well.<br>
>><br>
>> I'm not sure how to write tests that wouldn't be extremely brittle;<br>
>> ideas welcome.  The closest I have is my local testcase (attached,<br>
>> reduced from webkit), breaking some older revisions of clang when<br>
>> targeting AArch64.  Because of seemingly unrelated changes (I'm<br>
>> looking into it), the crashes don't happen anymore on trunk, but the<br>
>> problems are still there.<br>
>><br>
>> Thanks!<br>
>><br>
>> - Ahmed<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
</blockquote></div>