[PATCH] D58096: [LowerSwitch][AMDGPU] Do not handle impossible values

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin marked an inline comment as done.
rtereshin added inline comments.


================
Comment at: lib/Transforms/Utils/LowerSwitch.cpp:145
+  LazyValueInfo *LVI = &getAnalysis<LazyValueInfoWrapperPass>().getLVI();
+  LVI->disableDT();
+
----------------
arsenm wrote:
> This is strange looking. I'm not sure what it really means to disable the dominator tree
According to http://llvm.org/doxygen/classllvm_1_1LazyValueInfo.html#a5b29ad30fb31c6df2a3cbcefef8ae613 it "Disables use of the DominatorTree within LVI." As far as I can tell, the idea is that if LVI is allowed to use DT, DT needs to be valid at every point. As the LowerSwitch pass creates and deletes BBs as it goes, it will mean updating DT along the way using a DomTreeUpdater. Please see
```
commit 55da8a3a3e0af5afaa51f64c1385b2626c643317
Author: Brian M. Rzycki <brzycki at gmail.com>
Date:   Fri Feb 16 16:35:17 2018 +0000

    [JumpThreading] PR36133 enable/disable DominatorTree for LVI analysis

    Summary:
    The LazyValueInfo pass caches a copy of the DominatorTree when available.
    Whenever there are pending DominatorTree updates within JumpThreading's
    DeferredDominance object we cannot use the cached DT for LVI analysis.
    This commit adds the new methods enableDT() and disableDT() to LVI.
    JumpThreading also sets the appropriate usage model before calling LVI
    analysis methods.

    Fixes https://bugs.llvm.org/show_bug.cgi?id=36133

    Reviewers: sebpop, dberlin, kuhar

    Reviewed by: sebpop, kuhar

    Subscribers: uabelho, llvm-commits, aprantl, hiraditya, a.elovikov

    Differential Revision: https://reviews.llvm.org/D42717
```
for details.

I did't have time to investigate much neither how profitable it is to have the LVI using DT vs not using DT, nor what the compile time cost of using the updater, but from a quick glance it appeared that LVI only uses DT to do `isValidAssumeForContext` for assumptions from the AssumptionCache, which is kinda useless at least for us as we rarely if ever have any `llvm.assume` intrinsic calls on icmp's in the IR. Not to mention, `isValidAssumeForContext` can handle some of the cases even w/o a DT.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58096/new/

https://reviews.llvm.org/D58096