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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm accepted this revision.
arsenm added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: lib/Transforms/Utils/LowerSwitch.cpp:83
+    void getAnalysisUsage(AnalysisUsage &AU) const override {
+      AU.addRequired<LazyValueInfoWrapperPass>();
+    }
----------------
rtereshin wrote:
> arsenm wrote:
> > Does this need to use the default Function analysis usage?
> I'm not sure what should be declared preserved here.
> 
> I tried removing LowerSwitch from the following pipelines:
> ```
> -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900
> -O0 -mtriple=amdgcn-mesa-mesa3d -mcpu=bonaire
> -mtriple=amdgcn-mesa-mesa3d -mcpu=bonaire
> -march=r600 -mcpu=redwood
> ```
> In no case it saved a re-computation of any of the analyses.
> 
> Do you have any suggestions?
I know if you don't call the base class analysis usage it causes problems with MachineFunctions, but not sure about IR passes


================
Comment at: lib/Transforms/Utils/LowerSwitch.cpp:448-450
+  LLVM_DEBUG(dbgs() << "Clusterify finished. Total clusters: " << Cases.size()
+                    << ". Total non-default cases: " << NumSimpleCases << "\n");
+  LLVM_DEBUG(dbgs() << "Case clusters: " << Cases << "\n");
----------------
rtereshin wrote:
> arsenm wrote:
> > These can be merged into one LLVM_DEBUG (and single quotes around \n)
> > These can be merged into one LLVM_DEBUG
> 
> Sure, will do.
> 
> > and single quotes around \n
> 
> Why?
> ```
> grep -Eroh "<<\s*['\"]\\\?.['\"]" ./ --include='*.cpp' --include='*.h' --exclude-dir=build | grep -o "['\"]" | sort | uniq -c | sort -n
> ```
> returns
> ```
> 6492 '
> 12003 "
> ```
> (the example output of the first regex:
> ```
> << '"'
> << '['
> << '\n'
> << '\t'
> << '\n'
> << '\n'
> << "\n"
> << ":"
> << ":"
> << "\n"
> << "\n"
> << "\n"
> << "\n"
> << "\n"
> << "\n"
> << "\n"
> << " "
> ```
> the current folder includes TOT llvm w/o clang or other projects)
I don't remember where I saw this guideline, but apparently the character << is much cheaper for raw_ostream. I also just find using a string for a single character uglier.


================
Comment at: lib/Transforms/Utils/LowerSwitch.cpp:494
+    // Constraining the range of the value being switched over helps eliminating
+    // unreachable BBs and minimizing the number of add's newLeafBlock ends up
+    // emitting. Running CorrelatedValuePropagation after LowerSwitch isn't as
----------------
Typo add's


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58096