[PATCH] D142234: [ConstantRange] Handle Intrinsic::ctlz

[luxufan via Phabricator via llvm-commits]

StephenFan added inline comments.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:1686
+    // 1) Lower is zero, handling cases of kind [0, 1), [0, 2), etc.
+    // 2) Upper is zero, wrapped set, handling cases of kind [3, 0], etc.
+    // 3) Zero contained in a wrapped set, e.g., [3, 2), [3, 1), etc.
----------------
IIUC, is `[3, 0]` the same as the `[3, 1)` in case 3? 


================
Comment at: llvm/test/Analysis/LazyValueAnalysis/lvi-for-ctlz.ll:1
+; RUN: opt < %s -passes=jump-threading -print-lvi-after-jump-threading -disable-output 2>&1 | FileCheck %s
+
----------------
antoniofrighetto wrote:
> nikic wrote:
> > nikic wrote:
> > > Having an IR test is fine, but please do not test LVI debug output. Just check the resulting IR change using update_test_checks.py.
> > I don't think that as written, these tests really test anything. There needs to be a comparison involving the ctlz that can be folded away, or similar.
> I feel like a suitable test could be the following one:
> ```
> int lol(int b) {
>     if (b < 65536) {
>         int n = __builtin_clz(b);
>         if (n < 8)
>     	    return 0;
>         else
>             return 1;
>    }
>    return 2;
> }
> ```
> which now could be simplified in a `return (b < 65536) ? 1 : 2;`. However, as far as I understand this, CVP needs to be extended as well in order to be able to obtain the above constant folding. Is that correct?
Is the simplification to `return (b < 65536) ? 1:2;` correct? Since if `b` is negative, `clz` returns 0. 


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

https://reviews.llvm.org/D142234