[PATCH] D102116: [LoopIdiom] 'logical right-shift until zero' ('count active bits') "on steroids" idiom recognition.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:2605
+  BasicBlock *SuccessorBB = CurLoop->getExitBlock();
+  assert(LoopPreheaderBB && "There is only a single successor.");
+
----------------
craig.topper wrote:
> This should check SuccessorBB?
Indeed. Same with the previous transform.


================
Comment at: llvm/test/Transforms/LoopIdiom/X86/logical-right-shift-until-zero.ll:9-65
 ; Most basic pattern; Note that iff the shift amount is offset, said offsetting
 ; must not cause an overflow, but `add nsw` is fine.
 define i8 @p0(i8 %val, i8 %start, i8 %extraoffset) {
 ; CHECK-LABEL: @p0(
 ; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[VAL_NUMLEADINGZEROS:%.*]] = call i8 @llvm.ctlz.i8(i8 [[VAL:%.*]], i1 false)
+; CHECK-NEXT:    [[VAL_NUMACTIVEBITS:%.*]] = sub nuw nsw i8 8, [[VAL_NUMLEADINGZEROS]]
----------------
zhuhan0 wrote:
> I could be wrong but would this mis-compile if %nbits results in unsigned overflow? For example,
> ```
> %val = 0x10000000
> %start = 1
> %extraoffset.= 255
> ```
> Loop trip count is 8 before transformation but 1 after.
Could you please specify, for which bitwidth your counterexample is?
I'm going to guess i32, so we have
```
%iv = i32 1
%nbits = i32 256
%val.shifted = lshr i32 %val, 256
```
We then navigate to https://llvm.org/docs/LangRef.html#lshr-instruction
> If op2 is (statically or dynamically) equal to or larger than the number of bits in op1, this instruction returns a poison value.

So i'm not seeing a miscompile.

As i have said, i've verified each of the tests here with alive2, and they are all fine.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102116