<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Jun 20, 2018, at 12:20 PM, Benjamin Kramer <<a href="mailto:benny.kra@gmail.com" class="">benny.kra@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">I tried both to no avail. But I think I have the missing pieces of the<br class="">puzzle now:<br class=""><br class=""><ul class=""><li class="">We run LICM after JumpThreading and CorrelatedValuePropagation<br class=""></li><li class="">LICM depends on the order of children of a DomTreeNode<br class=""></li><li class="">When doing DDT inserts within one bulk edit the children are stored in a SmallDenseSet</li><li class="">SmallDenseSet iteration order is unspecified<br class=""></li></ul><br class="">I'm afraid I wont be able to find a test case for this (my example is<br class="">a 240M .ll file), but I'll take a stab at fixing DDT. Thanks for<br class="">rubber ducking :)</div></div></blockquote>Thanks for the nice explanation, it sounds very plausible!</div><div><br class=""></div><div>Michael<br class=""><blockquote type="cite" class=""><div class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Wed, Jun 20, 2018 at 8:43 PM Mikhail Zolotukhin <<a href="mailto:mzolotukhin@apple.com" class="">mzolotukhin@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I see. When you tried verification, did you use “-verify-dom-tree”, or did you put explicit "assert(DT->verify(…))" call into the code? If the former, then the latter approach might still catch the issue (if you insert this assert into the suspicious place, e.g. after a block deletion). If there is DDT involved, you’ll also need to run DDT->flush() before that.<br class="">
<br class="">
Michael<br class="">
<br class="">
> On Jun 20, 2018, at 9:04 AM, Benjamin Kramer <<a href="mailto:benny.kra@gmail.com" target="_blank" class="">benny.kra@gmail.com</a>> wrote:<br class="">
> <br class="">
> In JumpThreading. I don't think CVT is the problem here, it just<br class="">
> doesn't trigger a dominator recompute anymore.<br class="">
> On Wed, Jun 20, 2018 at 6:00 PM Michael Zolotukhin<br class="">
> <<a href="mailto:mzolotukhin@apple.com" target="_blank" class="">mzolotukhin@apple.com</a>> wrote:<br class="">
>> <br class="">
>> Are you talking about flushing DDT in JT or in CVT?<br class="">
>> <br class="">
>>> On Jun 20, 2018, at 8:31 AM, Benjamin Kramer <<a href="mailto:benny.kra@gmail.com" target="_blank" class="">benny.kra@gmail.com</a>> wrote:<br class="">
>>> <br class="">
>>> Sadly I can't share it and I don't know how to reduce it. Verification<br class="">
>>> doesn't catch it either. What works is flushing DeferredDominators<br class="">
>>> after we delete a block, which makes me think that there's something<br class="">
>>> wrong with the way we handle dead blocks.<br class="">
>>> On Wed, Jun 20, 2018 at 5:00 PM Michael Zolotukhin<br class="">
>>> <<a href="mailto:mzolotukhin@apple.com" target="_blank" class="">mzolotukhin@apple.com</a>> wrote:<br class="">
>>>> <br class="">
>>>> Hmm, that’s interesting. Could you please share your test-case so that I can poke at it too? Another thing to try would be to add explicit DT verification after JT - if recomputation helps, then the existing DT should be incorrect at that point.<br class="">
>>>> <br class="">
>>>> Michael<br class="">
>>>> <br class="">
>>>>> On Jun 20, 2018, at 6:11 AM, Benjamin Kramer <<a href="mailto:benny.kra@gmail.com" target="_blank" class="">benny.kra@gmail.com</a>> wrote:<br class="">
>>>>> <br class="">
>>>>> I'm seeing non-determinism after this change. Since this change is<br class="">
>>>>> fairly simple it's probably coming from earlier in the pipeline,<br class="">
>>>>> likely candidate being JumpThreading. Recalculating DT before<br class="">
>>>>> JumpThreading doesn't fix it, but recalculating after JumpThreading<br class="">
>>>>> removes the non-determinism.<br class="">
>>>>> <br class="">
>>>>> My test case is coming out of tfcompile and is huge and scary, any<br class="">
>>>>> ideas on how to track this down?<br class="">
>>>>> On Sat, Jun 16, 2018 at 9:01 PM Michael Zolotukhin via llvm-commits<br class="">
>>>>> <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>> wrote:<br class="">
>>>>>> <br class="">
>>>>>> Author: mzolotukhin<br class="">
>>>>>> Date: Sat Jun 16 11:57:31 2018<br class="">
>>>>>> New Revision: 334895<br class="">
>>>>>> <br class="">
>>>>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=334895&view=rev" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=334895&view=rev</a><br class="">
>>>>>> Log:<br class="">
>>>>>> CorrelatedValuePropagation: Preserve DT.<br class="">
>>>>>> <br class="">
>>>>>> Summary:<br class="">
>>>>>> We only modify CFG in a couple of places, and we can preserve DT there<br class="">
>>>>>> with a little effort.<br class="">
>>>>>> <br class="">
>>>>>> Reviewers: davide, vsk<br class="">
>>>>>> <br class="">
>>>>>> Subscribers: hiraditya, llvm-commits<br class="">
>>>>>> <br class="">
>>>>>> Differential Revision: <a href="https://reviews.llvm.org/D48059" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D48059</a><br class="">
>>>>>> <br class="">
>>>>>> Modified:<br class="">
>>>>>> llvm/trunk/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp<br class="">
>>>>>> llvm/trunk/test/Other/opt-O2-pipeline.ll<br class="">
>>>>>> llvm/trunk/test/Other/opt-O3-pipeline.ll<br class="">
>>>>>> llvm/trunk/test/Other/opt-Os-pipeline.ll<br class="">
>>>>>> <br class="">
>>>>>> Modified: llvm/trunk/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp<br class="">
>>>>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp?rev=334895&r1=334894&r2=334895&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp?rev=334895&r1=334894&r2=334895&view=diff</a><br class="">
>>>>>> ==============================================================================<br class="">
>>>>>> --- llvm/trunk/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp (original)<br class="">
>>>>>> +++ llvm/trunk/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp Sat Jun 16 11:57:31 2018<br class="">
>>>>>> @@ -82,6 +82,7 @@ namespace {<br class="">
>>>>>> AU.addRequired<DominatorTreeWrapperPass>();<br class="">
>>>>>> AU.addRequired<LazyValueInfoWrapperPass>();<br class="">
>>>>>> AU.addPreserved<GlobalsAAWrapperPass>();<br class="">
>>>>>> + AU.addPreserved<DominatorTreeWrapperPass>();<br class="">
>>>>>> }<br class="">
>>>>>> };<br class="">
>>>>>> <br class="">
>>>>>> @@ -306,7 +307,7 @@ static bool processCmp(CmpInst *C, LazyV<br class="">
>>>>>> /// that cannot fire no matter what the incoming edge can safely be removed. If<br class="">
>>>>>> /// a case fires on every incoming edge then the entire switch can be removed<br class="">
>>>>>> /// and replaced with a branch to the case destination.<br class="">
>>>>>> -static bool processSwitch(SwitchInst *SI, LazyValueInfo *LVI) {<br class="">
>>>>>> +static bool processSwitch(SwitchInst *SI, LazyValueInfo *LVI, DominatorTree *DT) {<br class="">
>>>>>> Value *Cond = SI->getCondition();<br class="">
>>>>>> BasicBlock *BB = SI->getParent();<br class="">
>>>>>> <br class="">
>>>>>> @@ -321,6 +322,10 @@ static bool processSwitch(SwitchInst *SI<br class="">
>>>>>> <br class="">
>>>>>> // Analyse each switch case in turn.<br class="">
>>>>>> bool Changed = false;<br class="">
>>>>>> + DenseMap<BasicBlock*, int> SuccessorsCount;<br class="">
>>>>>> + for (auto *Succ : successors(BB))<br class="">
>>>>>> + SuccessorsCount[Succ]++;<br class="">
>>>>>> +<br class="">
>>>>>> for (auto CI = SI->case_begin(), CE = SI->case_end(); CI != CE;) {<br class="">
>>>>>> ConstantInt *Case = CI->getCaseValue();<br class="">
>>>>>> <br class="">
>>>>>> @@ -355,7 +360,8 @@ static bool processSwitch(SwitchInst *SI<br class="">
>>>>>> <br class="">
>>>>>> if (State == LazyValueInfo::False) {<br class="">
>>>>>> // This case never fires - remove it.<br class="">
>>>>>> - CI->getCaseSuccessor()->removePredecessor(BB);<br class="">
>>>>>> + BasicBlock *Succ = CI->getCaseSuccessor();<br class="">
>>>>>> + Succ->removePredecessor(BB);<br class="">
>>>>>> CI = SI->removeCase(CI);<br class="">
>>>>>> CE = SI->case_end();<br class="">
>>>>>> <br class="">
>>>>>> @@ -365,6 +371,8 @@ static bool processSwitch(SwitchInst *SI<br class="">
>>>>>> <br class="">
>>>>>> ++NumDeadCases;<br class="">
>>>>>> Changed = true;<br class="">
>>>>>> + if (--SuccessorsCount[Succ] == 0)<br class="">
>>>>>> + DT->deleteEdge(BB, Succ);<br class="">
>>>>>> continue;<br class="">
>>>>>> }<br class="">
>>>>>> if (State == LazyValueInfo::True) {<br class="">
>>>>>> @@ -381,10 +389,14 @@ static bool processSwitch(SwitchInst *SI<br class="">
>>>>>> ++CI;<br class="">
>>>>>> }<br class="">
>>>>>> <br class="">
>>>>>> - if (Changed)<br class="">
>>>>>> + if (Changed) {<br class="">
>>>>>> // If the switch has been simplified to the point where it can be replaced<br class="">
>>>>>> // by a branch then do so now.<br class="">
>>>>>> - ConstantFoldTerminator(BB);<br class="">
>>>>>> + DeferredDominance DDT(*DT);<br class="">
>>>>>> + ConstantFoldTerminator(BB, /*DeleteDeadConditions = */ false,<br class="">
>>>>>> + /*TLI = */ nullptr, &DDT);<br class="">
>>>>>> + DDT.flush();<br class="">
>>>>>> + }<br class="">
>>>>>> <br class="">
>>>>>> return Changed;<br class="">
>>>>>> }<br class="">
>>>>>> @@ -722,7 +734,7 @@ static bool runImpl(Function &F, LazyVal<br class="">
>>>>>> Instruction *Term = BB->getTerminator();<br class="">
>>>>>> switch (Term->getOpcode()) {<br class="">
>>>>>> case Instruction::Switch:<br class="">
>>>>>> - BBChanged |= processSwitch(cast<SwitchInst>(Term), LVI);<br class="">
>>>>>> + BBChanged |= processSwitch(cast<SwitchInst>(Term), LVI, DT);<br class="">
>>>>>> break;<br class="">
>>>>>> case Instruction::Ret: {<br class="">
>>>>>> auto *RI = cast<ReturnInst>(Term);<br class="">
>>>>>> @@ -767,5 +779,6 @@ CorrelatedValuePropagationPass::run(Func<br class="">
>>>>>> return PreservedAnalyses::all();<br class="">
>>>>>> PreservedAnalyses PA;<br class="">
>>>>>> PA.preserve<GlobalsAA>();<br class="">
>>>>>> + PA.preserve<DominatorTreeAnalysis>();<br class="">
>>>>>> return PA;<br class="">
>>>>>> }<br class="">
>>>>>> <br class="">
>>>>>> Modified: llvm/trunk/test/Other/opt-O2-pipeline.ll<br class="">
>>>>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/opt-O2-pipeline.ll?rev=334895&r1=334894&r2=334895&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/opt-O2-pipeline.ll?rev=334895&r1=334894&r2=334895&view=diff</a><br class="">
>>>>>> ==============================================================================<br class="">
>>>>>> --- llvm/trunk/test/Other/opt-O2-pipeline.ll (original)<br class="">
>>>>>> +++ llvm/trunk/test/Other/opt-O2-pipeline.ll Sat Jun 16 11:57:31 2018<br class="">
>>>>>> @@ -145,7 +145,6 @@<br class="">
>>>>>> ; CHECK-NEXT: Lazy Value Information Analysis<br class="">
>>>>>> ; CHECK-NEXT: Jump Threading<br class="">
>>>>>> ; CHECK-NEXT: Value Propagation<br class="">
>>>>>> -; CHECK-NEXT: Dominator Tree Construction<br class="">
>>>>>> ; CHECK-NEXT: Basic Alias Analysis (stateless AA impl)<br class="">
>>>>>> ; CHECK-NEXT: Function Alias Analysis Results<br class="">
>>>>>> ; CHECK-NEXT: Memory Dependence Analysis<br class="">
>>>>>> <br class="">
>>>>>> Modified: llvm/trunk/test/Other/opt-O3-pipeline.ll<br class="">
>>>>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/opt-O3-pipeline.ll?rev=334895&r1=334894&r2=334895&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/opt-O3-pipeline.ll?rev=334895&r1=334894&r2=334895&view=diff</a><br class="">
>>>>>> ==============================================================================<br class="">
>>>>>> --- llvm/trunk/test/Other/opt-O3-pipeline.ll (original)<br class="">
>>>>>> +++ llvm/trunk/test/Other/opt-O3-pipeline.ll Sat Jun 16 11:57:31 2018<br class="">
>>>>>> @@ -149,7 +149,6 @@<br class="">
>>>>>> ; CHECK-NEXT: Lazy Value Information Analysis<br class="">
>>>>>> ; CHECK-NEXT: Jump Threading<br class="">
>>>>>> ; CHECK-NEXT: Value Propagation<br class="">
>>>>>> -; CHECK-NEXT: Dominator Tree Construction<br class="">
>>>>>> ; CHECK-NEXT: Basic Alias Analysis (stateless AA impl)<br class="">
>>>>>> ; CHECK-NEXT: Function Alias Analysis Results<br class="">
>>>>>> ; CHECK-NEXT: Memory Dependence Analysis<br class="">
>>>>>> <br class="">
>>>>>> Modified: llvm/trunk/test/Other/opt-Os-pipeline.ll<br class="">
>>>>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/opt-Os-pipeline.ll?rev=334895&r1=334894&r2=334895&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/opt-Os-pipeline.ll?rev=334895&r1=334894&r2=334895&view=diff</a><br class="">
>>>>>> ==============================================================================<br class="">
>>>>>> --- llvm/trunk/test/Other/opt-Os-pipeline.ll (original)<br class="">
>>>>>> +++ llvm/trunk/test/Other/opt-Os-pipeline.ll Sat Jun 16 11:57:31 2018<br class="">
>>>>>> @@ -131,7 +131,6 @@<br class="">
>>>>>> ; CHECK-NEXT: Lazy Value Information Analysis<br class="">
>>>>>> ; CHECK-NEXT: Jump Threading<br class="">
>>>>>> ; CHECK-NEXT: Value Propagation<br class="">
>>>>>> -; CHECK-NEXT: Dominator Tree Construction<br class="">
>>>>>> ; CHECK-NEXT: Basic Alias Analysis (stateless AA impl)<br class="">
>>>>>> ; CHECK-NEXT: Function Alias Analysis Results<br class="">
>>>>>> ; CHECK-NEXT: Memory Dependence Analysis<br class="">
>>>>>> <br class="">
>>>>>> <br class="">
>>>>>> _______________________________________________<br class="">
>>>>>> llvm-commits mailing list<br class="">
>>>>>> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a><br class="">
>>>>>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class="">
>>>> <br class="">
>> <br class="">
<br class="">
</blockquote></div>
</div></blockquote></div><br class=""></body></html>