[PATCH] D27327: [InstCombine] Fix to switch narrowing

[Gerolf Hoflehner via Phabricator via llvm-commits]

Gerolf added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2276
-      Constant *LHS = CaseVal;
-      if (TruncCond) {
-        LHS = LeadingKnownZeros
----------------
hans wrote:
> What happened to this part?
Gone as part of the fix: the condition expression a+const is evaluated at full width before the case constants are inspected. 


================
Comment at: test/Transforms/InstCombine/narrow-switch.ll:108
 ; ALL-LABEL: @trunc64to59(
-; ALL:         switch i59
-; ALL-NEXT:    i59 0, label %sw.bb1
-; ALL-NEXT:    i59 18717182647723699, label %sw.bb2
+; ALL:         switch i61
+; ALL-NEXT:    i61 0, label %sw.bb1
----------------
hans wrote:
> Why does this need to be i61 now, and should the test be renamed to reflect that?
The original code evaluates the case constants.
+ It recognizes they can be truncated to 61b.
+ It truncates the case values to 61b
+ It truncates the cond value to 61b
+ It checks the condition a+const in 61b and adjusts the case value - const to 61b. 
+ It returns SI, so switch narrowing is invoked again and now finds the case values to be 59b. It is this double adjustment of the case values that potentially can lead to a bug. Obviously the test cases in narrow-switch.ll did not catch it.

The fix evaluates a+const in 64b and changes the case value to value -const (still 64b). Then it finds the case values to be 61b , and adjusts with of the values and cond accordingly. 


https://reviews.llvm.org/D27327