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

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:648
     }
-
+    LVI->enableDT();
     return !Result.empty();
----------------
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.


https://reviews.llvm.org/D42717