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

[James Molloy via Phabricator via llvm-commits]

jmolloy added inline comments.


================
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
----------------
shawnl wrote:
> 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().
I'm not sure I agree with that, or at least I'm missing something.

Given a sorted array A of integers with popcount 1 (single bit set), I believe:

  order(A) == order(cttz(A))  //  cttz does not change the order.
  order(A) == reverse(order(ctlz(A))) // Conversely ctlz reverses the order.

If you need to handle the zero value too, then I would have thought cttz would barrel-shift as you describe and ctlz would barrel-shift the reversed order.

My comment above was based on the assumption that you're not allowing zero to be passed to ctlz/cttz. Given you are, just keep on using llvm::sort.


================
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())
----------------
shawnl wrote:
> 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.
We can fix that. Please keep using SmallVector and we can work through the difficulties.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61160