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

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin marked 3 inline comments as done.
rtereshin added a comment.

Thank you!



================
Comment at: lib/Transforms/Utils/LowerSwitch.cpp:83
+    void getAnalysisUsage(AnalysisUsage &AU) const override {
+      AU.addRequired<LazyValueInfoWrapperPass>();
+    }
----------------
arsenm wrote:
> 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
Ah, true, I completely misunderstood you. AFAIK, for function passes the base class usage is empty and doesn't do anything.


================
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");
----------------
arsenm wrote:
> 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.
I guess it depends on the tooling used to build it and operating system, And maybe on the version of LLVM as well.

I checked on macOS (llvm built by clang) as soon as you mentioned this, just printing a million new lines both ways. The time is exactly the same.

As for personal preferences, I guess they are just that. I find it a few keystrokes easier to change between "\n", ".\n", ":\n", " " and whatnot when they are all strings.


================
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
----------------
arsenm wrote:
> Typo add's
Not sure what do you mean. By add's I meant `add` instructions. I will replace it with that to avoid confusion, thanks!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58096