[PATCH] D52707: Switch optimization in IR for known maximum switch values

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

Sorry for the slow response time. I'm back from vacation now.



================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4850
+// case statements and direct them to the default block.  Then mark the default
+// destination as unreachable.
+static bool widenSwitchRangeToCoverAllValues(SwitchInst *SI,
----------------
The function description is still a bit confusing. I understand what you mean by "remap the known default values into individual case statements", but requires some thinking to understand.

I would suggest rephrasing it as "Possibly widen the switch by adding more cases so that all possible input values are covered, making the default case unreachable."


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4860
+  bool UnreachableDefault =
+      isa<UnreachableInst>(SI->getDefaultDest()->getFirstNonPHIOrDbg());
+
----------------
Since computeKnownBits takes some work to do, I would move the check and return for unreachable default right to the start of the function so we don't do any unnecessary work in that case.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4862
+
+  // We only care for non-negative values.  The number of minimum leading
+  // zeros plus the number of maximum trailing ones should be equal to the
----------------
Why do we only care about non-negative values?

And more importantly, why the check for minimum number of leading zeros and maximum trailing ones... just reading that, it sounds like it's always true? Is there any situation when it's not true?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4883
+  // statements.
+  if (MaxSwitchValue.ugt(NumCases)  &&
+      SI->getNumCases() > 0  &&
----------------
Why does MaxSwitchValue have to be greater than the number of cases?

I'm not sure I'm following the logic here actually. Also, could we have cases that are greater than MaxSwitchValue, or will some other pass have removed them?


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

https://reviews.llvm.org/D52707