[PATCH] D61159: [SimplifyCFG] Run ReduceSwitchRange unconditionally, generalize

[James Molloy via Phabricator via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.

Thanks! Much easier to review this time.



================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5514
+  // This is also useful when using the LowerSwitch transform, but not with
+  // no few cases.
   if (SI->getNumCases() < 4)
----------------
no -> so


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5525
+  bool MadeChanges = false;
+  // We must first look find the best start point.
+  uint64_t BestDistance = APInt::getMaxValue(CondTy->getIntegerBitWidth()).getLimitedValue() -
----------------
Can you mention why we're doing this? A naive reader may wonder why this might possibly return any value other than Values.back() - Values.front() + 1 (of course, a counterexample is the signed zero-crossing case mentioned in the diffbase).


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5529
+  unsigned BestIndex = 0;
+  for (unsigned i = 1;i != Values.size();i++) {
+    if (Values[i] - Values[i-1] > BestDistance) {
----------------
Please use capitals for induction variables, calculate the end value once, and use preincrement as per style guide:

  for (unsigned I = 1, E = Values.size(); I != E; ++I) {


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5537
+  // Now transform the values such that they start at zero and ascend.
+  if ((BestDistance > SubThreshold) &&
+    (BestIndex != 0 || (Values[0] >= SubThreshold))) {
----------------
Is this heuristic correct? From my understanding, SubThreshold is the number of table entries elided. BestDistance is the wrap distance.

But the number of table entries elided isn't the wrap distance, it's the Base that you calculate below (because by default we start counting from zero).

Am I wrong? I think this should be if (Base > SubThreshold ...)


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5538
+  if ((BestDistance > SubThreshold) &&
+    (BestIndex != 0 || (Values[0] >= SubThreshold))) {
+    Base = Values[BestIndex];
----------------
And if I'm right above, this whole clause goes away and the entire if becomes:

  if (Values[BestIndex] >= SubThreshold)



================
Comment at: test/Transforms/SimplifyCFG/X86/disable-lookup-table.ll:55
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[SWITCH_TABLEIDX:%.*]] = sub i32 [[C:%.*]], 42
-; CHECK-NEXT:    [[TMP0:%.*]] = icmp ult i32 [[SWITCH_TABLEIDX]], 4
-; CHECK-NEXT:    br i1 [[TMP0]], label [[SWITCH_LOOKUP:%.*]], label [[RETURN:%.*]]
+; CHECK-NEXT:    [[TMP0:%.*]] = sub i32 [[C:%.*]], 42
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp ult i32 [[TMP0]], 4
----------------
Can you please name the CreateSub() you added so it's not just a temporary? like "switch.reducerange" or something?


================
Comment at: test/Transforms/SimplifyCFG/X86/switch-table-bug.ll:20
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[SWITCH_TABLEIDX:%.*]] = sub i2 [[TMP0:%.*]], -2
-; CHECK-NEXT:    [[SWITCH_TABLEIDX_ZEXT:%.*]] = zext i2 [[SWITCH_TABLEIDX]] to i3
-; CHECK-NEXT:    [[SWITCH_GEP:%.*]] = getelementptr inbounds [4 x i64], [4 x i64]* @switch.table._TFO6reduce1E5toRawfS0_FT_Si, i32 0, i3 [[SWITCH_TABLEIDX_ZEXT]]
-; CHECK-NEXT:    [[SWITCH_LOAD:%.*]] = load i64, i64* [[SWITCH_GEP]]
-; CHECK-NEXT:    ret i64 [[SWITCH_LOAD]]
+; CHECK-NEXT:    [[SWITCH_IDX_CAST:%.*]] = zext i2 [[TMP0:%.*]] to i64
+; CHECK-NEXT:    ret i64 [[SWITCH_IDX_CAST]]
----------------
I particularly like this simplification :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61159