[PATCH] D42717: [JumpThreading] sync DT for LVI analysis (PR 36133)

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:648
     }
-
+    LVI->enableDT();
     return !Result.empty();
----------------
brzycki wrote:
> a.elovikov wrote:
> > Isn't it strange that we enable LVI without flushing DDT first? Even if it's ok now I'm afraid it might become a source of nasty bugs in the future.
> > 
> > And related question - would it be better to integrate the LVI->disableDT/enableDT calls into the DDT itself (or a wrapper around it)? So that any deferred update to the DT would automatically make DT disabled in LVI and every flush would make it enabled?
> Hello @a.elovikov,
> 
> I chose to enable/disable around each LVI analysis call in the case that future patches call `DDT->flush()` within JumpThreading. I could also make this call and feel comfortable with its implications:
> 
> ```
> if (DDT->pending())
>   LVI->disableDT();
> else
>   LVI->enableDT();
> // LVI analysis begins...
> ```
> 
> As for adding LVI into the DDT class itself, that approach ended up being full of problems. The LazyValue class doesn't actually have the code for enableDT()/disableDT() in the header like traditional LLVM classes. Because the class is broken into two for caching on-demand I get compile-time errors with LLVM unable to find the actual implementation of the disableDT()/enableDT() functions at link time:
> ```
> lib/IR/CMakeFiles/LLVMCore.dir/Dominators.cpp.o: In function `llvm::DeferredDominance::applyUpdate(llvm::DomTreeBuilder::UpdateKind, llvm::BasicBlock*, llvm::BasicBlock*)':
> /work/brzycki/D42717/llvm-project/llvm/lib/IR/Dominators.cpp:569: undefined reference to `llvm::LazyValueInfo::disableDT()'
> lib/IR/CMakeFiles/LLVMCore.dir/Dominators.cpp.o: In function `llvm::DeferredDominance::flush()':
> /work/brzycki/D42717/llvm-project/llvm/lib/IR/Dominators.cpp:464: undefined reference to `llvm::LazyValueInfo::enableDT()'
> lib/IR/CMakeFiles/LLVMCore.dir/Dominators.cpp.o: In function `llvm::DeferredDominance::recalculate(llvm::Function&)':
> /work/brzycki/D42717/llvm-project/llvm/lib/IR/Dominators.cpp:481: undefined reference to `llvm::LazyValueInfo::enableDT()'
> clang: error: linker command failed with exit code 1 (use -v to see invocation)
> ```
> 
> If you know of a way to fix these errors I'll give it another try.
Hey @a.elovikov ,

You really don't want DDT to know things about passes.  IMHO, if you were going to go that route, LVI checks whether DT is null, and so you could wrap this all in a "UpdatedDTOrNull" object that, when cast to a DominatorTree *, returned the tree if it was up to date, or a null pointer if it was not.

That *looks* nicer code wise.
But one advantage to the way Brian is doing it is that it is easily verifiable and doesn't hide where and when LVI has a disabled DT.

When we hide stuff like that, it tends to regress and nobody notices for a while.




https://reviews.llvm.org/D42717