[PATCH] D61160: [SimplifyCFG] Implement the suggested ctlz transform

[Shawn Landden via Phabricator via llvm-commits]

shawnl marked 6 inline comments as done.
shawnl added inline comments.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5500
+  // emit bit width. If not handled this can result in the transform
+  // being applied multiple times. We can either rotate bit width back to zero,
+  // by doing an xor or the most significant bit, followed by a rotate-left 1,
----------------
jmolloy wrote:
> I would avoid talking about the solution you didn't implement. Checking for zero input (which your popcount does) is a totally reasonable and understandable thing (and much more understandable than trying to grok the xor/rotate explanation to then find it isn't used :D )
> which your popcount does

It does not. It only checks if the MAXIMUM is 1.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5515
+    return false;
+  // TODO a single instruction can be saved on x86 by using cttz over ctlz
+  // when only ctlz results in a subtraction instruction being added
----------------
jmolloy wrote:
> This would be a pattern for instcombine to pick up. We shouldn't have to worry about this here.
Yes we do, because InstCombine does not know how to rearrange case statements.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5519
+    V = APInt(BitWidth, V).countLeadingZeros();
+  // 0 will suddenly become the largest (BitWidth), so we need to sort again.
+  // Sorting is OK because we either make a transform on all numbers, or do
----------------
jmolloy wrote:
> Isn't the order exactly reversed?
> 
>   llvm::reverse(Values);
Nope, the first becomes the last. I guess I could use swap() and move().


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5553
   // We organize the range to start from 0, if it is not already close.
-  SmallVector<uint64_t,4> Values;
+  std::vector<uint64_t> Values;
   for (auto &C : SI->cases())
----------------
jmolloy wrote:
> Please keep using SmallVector.
I was having problems when I tried to modify the reference to SmallVectorImpl that I did not know how to fix.


================
Comment at: test/Transforms/SimplifyCFG/rangereduce.ll:121
 ; CHECK-NEXT:    [[TMP1:%.*]] = sub i32 [[A:%.*]], 97
-; CHECK-NEXT:    [[TMP2:%.*]] = call i32 @llvm.fshr.i32(i32 [[TMP1]], i32 [[TMP1]], i32 0)
-; CHECK-NEXT:    switch i32 [[TMP2]], label [[DEF:%.*]] [
----------------
jmolloy wrote:
> Your previous patch is clearly bogus; this is a shift by zero.
shifts by zero are well defined, and get cleaned up by the optimizer, but I agree it didn't need to be inserted.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61160