[llvm] r334895 - CorrelatedValuePropagation: Preserve DT.
Mikhail Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 20 12:32:51 PDT 2018
> On Jun 20, 2018, at 12:20 PM, Benjamin Kramer <benny.kra at gmail.com> wrote:
>
> I tried both to no avail. But I think I have the missing pieces of the
> puzzle now:
>
> We run LICM after JumpThreading and CorrelatedValuePropagation
> LICM depends on the order of children of a DomTreeNode
> When doing DDT inserts within one bulk edit the children are stored in a SmallDenseSet
> SmallDenseSet iteration order is unspecified
>
> I'm afraid I wont be able to find a test case for this (my example is
> a 240M .ll file), but I'll take a stab at fixing DDT. Thanks for
> rubber ducking :)
Thanks for the nice explanation, it sounds very plausible!
Michael
>
>
> On Wed, Jun 20, 2018 at 8:43 PM Mikhail Zolotukhin <mzolotukhin at apple.com <mailto:mzolotukhin at apple.com>> wrote:
> 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.
>
> Michael
>
> > On Jun 20, 2018, at 9:04 AM, Benjamin Kramer <benny.kra at gmail.com <mailto:benny.kra at gmail.com>> wrote:
> >
> > In JumpThreading. I don't think CVT is the problem here, it just
> > doesn't trigger a dominator recompute anymore.
> > On Wed, Jun 20, 2018 at 6:00 PM Michael Zolotukhin
> > <mzolotukhin at apple.com <mailto:mzolotukhin at apple.com>> wrote:
> >>
> >> Are you talking about flushing DDT in JT or in CVT?
> >>
> >>> On Jun 20, 2018, at 8:31 AM, Benjamin Kramer <benny.kra at gmail.com <mailto:benny.kra at gmail.com>> wrote:
> >>>
> >>> Sadly I can't share it and I don't know how to reduce it. Verification
> >>> doesn't catch it either. What works is flushing DeferredDominators
> >>> after we delete a block, which makes me think that there's something
> >>> wrong with the way we handle dead blocks.
> >>> On Wed, Jun 20, 2018 at 5:00 PM Michael Zolotukhin
> >>> <mzolotukhin at apple.com <mailto:mzolotukhin at apple.com>> wrote:
> >>>>
> >>>> 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.
> >>>>
> >>>> Michael
> >>>>
> >>>>> On Jun 20, 2018, at 6:11 AM, Benjamin Kramer <benny.kra at gmail.com <mailto:benny.kra at gmail.com>> wrote:
> >>>>>
> >>>>> I'm seeing non-determinism after this change. Since this change is
> >>>>> fairly simple it's probably coming from earlier in the pipeline,
> >>>>> likely candidate being JumpThreading. Recalculating DT before
> >>>>> JumpThreading doesn't fix it, but recalculating after JumpThreading
> >>>>> removes the non-determinism.
> >>>>>
> >>>>> My test case is coming out of tfcompile and is huge and scary, any
> >>>>> ideas on how to track this down?
> >>>>> On Sat, Jun 16, 2018 at 9:01 PM Michael Zolotukhin via llvm-commits
> >>>>> <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
> >>>>>>
> >>>>>> Author: mzolotukhin
> >>>>>> Date: Sat Jun 16 11:57:31 2018
> >>>>>> New Revision: 334895
> >>>>>>
> >>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=334895&view=rev <http://llvm.org/viewvc/llvm-project?rev=334895&view=rev>
> >>>>>> Log:
> >>>>>> CorrelatedValuePropagation: Preserve DT.
> >>>>>>
> >>>>>> Summary:
> >>>>>> We only modify CFG in a couple of places, and we can preserve DT there
> >>>>>> with a little effort.
> >>>>>>
> >>>>>> Reviewers: davide, vsk
> >>>>>>
> >>>>>> Subscribers: hiraditya, llvm-commits
> >>>>>>
> >>>>>> Differential Revision: https://reviews.llvm.org/D48059 <https://reviews.llvm.org/D48059>
> >>>>>>
> >>>>>> Modified:
> >>>>>> llvm/trunk/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
> >>>>>> llvm/trunk/test/Other/opt-O2-pipeline.ll
> >>>>>> llvm/trunk/test/Other/opt-O3-pipeline.ll
> >>>>>> llvm/trunk/test/Other/opt-Os-pipeline.ll
> >>>>>>
> >>>>>> Modified: llvm/trunk/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
> >>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp?rev=334895&r1=334894&r2=334895&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp?rev=334895&r1=334894&r2=334895&view=diff>
> >>>>>> ==============================================================================
> >>>>>> --- llvm/trunk/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp (original)
> >>>>>> +++ llvm/trunk/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp Sat Jun 16 11:57:31 2018
> >>>>>> @@ -82,6 +82,7 @@ namespace {
> >>>>>> AU.addRequired<DominatorTreeWrapperPass>();
> >>>>>> AU.addRequired<LazyValueInfoWrapperPass>();
> >>>>>> AU.addPreserved<GlobalsAAWrapperPass>();
> >>>>>> + AU.addPreserved<DominatorTreeWrapperPass>();
> >>>>>> }
> >>>>>> };
> >>>>>>
> >>>>>> @@ -306,7 +307,7 @@ static bool processCmp(CmpInst *C, LazyV
> >>>>>> /// that cannot fire no matter what the incoming edge can safely be removed. If
> >>>>>> /// a case fires on every incoming edge then the entire switch can be removed
> >>>>>> /// and replaced with a branch to the case destination.
> >>>>>> -static bool processSwitch(SwitchInst *SI, LazyValueInfo *LVI) {
> >>>>>> +static bool processSwitch(SwitchInst *SI, LazyValueInfo *LVI, DominatorTree *DT) {
> >>>>>> Value *Cond = SI->getCondition();
> >>>>>> BasicBlock *BB = SI->getParent();
> >>>>>>
> >>>>>> @@ -321,6 +322,10 @@ static bool processSwitch(SwitchInst *SI
> >>>>>>
> >>>>>> // Analyse each switch case in turn.
> >>>>>> bool Changed = false;
> >>>>>> + DenseMap<BasicBlock*, int> SuccessorsCount;
> >>>>>> + for (auto *Succ : successors(BB))
> >>>>>> + SuccessorsCount[Succ]++;
> >>>>>> +
> >>>>>> for (auto CI = SI->case_begin(), CE = SI->case_end(); CI != CE;) {
> >>>>>> ConstantInt *Case = CI->getCaseValue();
> >>>>>>
> >>>>>> @@ -355,7 +360,8 @@ static bool processSwitch(SwitchInst *SI
> >>>>>>
> >>>>>> if (State == LazyValueInfo::False) {
> >>>>>> // This case never fires - remove it.
> >>>>>> - CI->getCaseSuccessor()->removePredecessor(BB);
> >>>>>> + BasicBlock *Succ = CI->getCaseSuccessor();
> >>>>>> + Succ->removePredecessor(BB);
> >>>>>> CI = SI->removeCase(CI);
> >>>>>> CE = SI->case_end();
> >>>>>>
> >>>>>> @@ -365,6 +371,8 @@ static bool processSwitch(SwitchInst *SI
> >>>>>>
> >>>>>> ++NumDeadCases;
> >>>>>> Changed = true;
> >>>>>> + if (--SuccessorsCount[Succ] == 0)
> >>>>>> + DT->deleteEdge(BB, Succ);
> >>>>>> continue;
> >>>>>> }
> >>>>>> if (State == LazyValueInfo::True) {
> >>>>>> @@ -381,10 +389,14 @@ static bool processSwitch(SwitchInst *SI
> >>>>>> ++CI;
> >>>>>> }
> >>>>>>
> >>>>>> - if (Changed)
> >>>>>> + if (Changed) {
> >>>>>> // If the switch has been simplified to the point where it can be replaced
> >>>>>> // by a branch then do so now.
> >>>>>> - ConstantFoldTerminator(BB);
> >>>>>> + DeferredDominance DDT(*DT);
> >>>>>> + ConstantFoldTerminator(BB, /*DeleteDeadConditions = */ false,
> >>>>>> + /*TLI = */ nullptr, &DDT);
> >>>>>> + DDT.flush();
> >>>>>> + }
> >>>>>>
> >>>>>> return Changed;
> >>>>>> }
> >>>>>> @@ -722,7 +734,7 @@ static bool runImpl(Function &F, LazyVal
> >>>>>> Instruction *Term = BB->getTerminator();
> >>>>>> switch (Term->getOpcode()) {
> >>>>>> case Instruction::Switch:
> >>>>>> - BBChanged |= processSwitch(cast<SwitchInst>(Term), LVI);
> >>>>>> + BBChanged |= processSwitch(cast<SwitchInst>(Term), LVI, DT);
> >>>>>> break;
> >>>>>> case Instruction::Ret: {
> >>>>>> auto *RI = cast<ReturnInst>(Term);
> >>>>>> @@ -767,5 +779,6 @@ CorrelatedValuePropagationPass::run(Func
> >>>>>> return PreservedAnalyses::all();
> >>>>>> PreservedAnalyses PA;
> >>>>>> PA.preserve<GlobalsAA>();
> >>>>>> + PA.preserve<DominatorTreeAnalysis>();
> >>>>>> return PA;
> >>>>>> }
> >>>>>>
> >>>>>> Modified: llvm/trunk/test/Other/opt-O2-pipeline.ll
> >>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/opt-O2-pipeline.ll?rev=334895&r1=334894&r2=334895&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/opt-O2-pipeline.ll?rev=334895&r1=334894&r2=334895&view=diff>
> >>>>>> ==============================================================================
> >>>>>> --- llvm/trunk/test/Other/opt-O2-pipeline.ll (original)
> >>>>>> +++ llvm/trunk/test/Other/opt-O2-pipeline.ll Sat Jun 16 11:57:31 2018
> >>>>>> @@ -145,7 +145,6 @@
> >>>>>> ; CHECK-NEXT: Lazy Value Information Analysis
> >>>>>> ; CHECK-NEXT: Jump Threading
> >>>>>> ; CHECK-NEXT: Value Propagation
> >>>>>> -; CHECK-NEXT: Dominator Tree Construction
> >>>>>> ; CHECK-NEXT: Basic Alias Analysis (stateless AA impl)
> >>>>>> ; CHECK-NEXT: Function Alias Analysis Results
> >>>>>> ; CHECK-NEXT: Memory Dependence Analysis
> >>>>>>
> >>>>>> Modified: llvm/trunk/test/Other/opt-O3-pipeline.ll
> >>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/opt-O3-pipeline.ll?rev=334895&r1=334894&r2=334895&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/opt-O3-pipeline.ll?rev=334895&r1=334894&r2=334895&view=diff>
> >>>>>> ==============================================================================
> >>>>>> --- llvm/trunk/test/Other/opt-O3-pipeline.ll (original)
> >>>>>> +++ llvm/trunk/test/Other/opt-O3-pipeline.ll Sat Jun 16 11:57:31 2018
> >>>>>> @@ -149,7 +149,6 @@
> >>>>>> ; CHECK-NEXT: Lazy Value Information Analysis
> >>>>>> ; CHECK-NEXT: Jump Threading
> >>>>>> ; CHECK-NEXT: Value Propagation
> >>>>>> -; CHECK-NEXT: Dominator Tree Construction
> >>>>>> ; CHECK-NEXT: Basic Alias Analysis (stateless AA impl)
> >>>>>> ; CHECK-NEXT: Function Alias Analysis Results
> >>>>>> ; CHECK-NEXT: Memory Dependence Analysis
> >>>>>>
> >>>>>> Modified: llvm/trunk/test/Other/opt-Os-pipeline.ll
> >>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/opt-Os-pipeline.ll?rev=334895&r1=334894&r2=334895&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/opt-Os-pipeline.ll?rev=334895&r1=334894&r2=334895&view=diff>
> >>>>>> ==============================================================================
> >>>>>> --- llvm/trunk/test/Other/opt-Os-pipeline.ll (original)
> >>>>>> +++ llvm/trunk/test/Other/opt-Os-pipeline.ll Sat Jun 16 11:57:31 2018
> >>>>>> @@ -131,7 +131,6 @@
> >>>>>> ; CHECK-NEXT: Lazy Value Information Analysis
> >>>>>> ; CHECK-NEXT: Jump Threading
> >>>>>> ; CHECK-NEXT: Value Propagation
> >>>>>> -; CHECK-NEXT: Dominator Tree Construction
> >>>>>> ; CHECK-NEXT: Basic Alias Analysis (stateless AA impl)
> >>>>>> ; CHECK-NEXT: Function Alias Analysis Results
> >>>>>> ; CHECK-NEXT: Memory Dependence Analysis
> >>>>>>
> >>>>>>
> >>>>>> _______________________________________________
> >>>>>> llvm-commits mailing list
> >>>>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> >>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
> >>>>
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180620/57876404/attachment.html>
More information about the llvm-commits
mailing list