[PATCH] D113291: [AggressiveInstCombine] Lower Table Based CTTZ

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/lower-table-based-cttz-basics.ll:123
+
+define i32 @ctz7(i32 %x) {
+; CHECK-LABEL: @ctz7(
----------------
This is identical to the previous test, so it is not adding value here.

I realize that the source example it intended to model is slightly different. If you want to verify that we end up with cttz from the IR produced by clang, then I'd recommend adding a file to test/Transforms/PhaseOrdering and "RUN -O3". 

When I create tests like that, I grab the unoptimized IR using "clang -S -o - -emit-llvm -Xclang -disable-llvm-optzns". Then reduce it by running it through "opt -mem2reg", so it's not completely full of junk IR.


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/lower-table-based-cttz-non-argument-value.ll:78
+  call void @llvm.lifetime.start.p0i8(i64 4, i8* nonnull %0)
+  %call = call i32 (i8*, ...) @__isoc99_scanf(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @.str, i64 0, i64 0), i32* nonnull %x)
+  %1 = load i32, i32* %x, align 4
----------------
dmgreen wrote:
> It is better to pass x as a parameter, although I'm not sure it matter much where x comes from for the rest of the pattern.
Right - as far as this patch is concerned, this is identical to the previous test, so it shouldn't be here.

See my earlier comment about PhaseOrdering tests if we want more end-to-end coverage for `opt -O3`.


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

https://reviews.llvm.org/D113291