[llvm] r334895 - CorrelatedValuePropagation: Preserve DT.
Mikhail Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 20 11:43:01 PDT 2018
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> 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> 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> 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> 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> 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> 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
>>>>>> 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
>>>>>>
>>>>>> 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
>>>>>> ==============================================================================
>>>>>> --- 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
>>>>>> ==============================================================================
>>>>>> --- 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
>>>>>> ==============================================================================
>>>>>> --- 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
>>>>>> ==============================================================================
>>>>>> --- 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
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>
More information about the llvm-commits
mailing list