[PATCH] D136668: [clang][dataflow] Add initial sign analysis

Wed Oct 26 06:08:19 PDT 2022

martong marked 13 inline comments as done.
martong added a comment.

In D136668#3883241 <https://reviews.llvm.org/D136668#3883241>, @ymandel wrote:

> Nice!!  Just nits.
>
> At a high level, I'm a little concerned about this as a demo, since I wouldn't recommend this implementation in practice  (for various reasons, e.g. I would encode with 2 booleans since there are only 3 values). But, I think this approach has the advantage of clarity over optimized approaches. It might be worth pointing this out in comments at places where you made a decision for purposes of clarity/readability, if any come to mind.

Okay, thanks again for the review! I've changed the comments for the file, where I describe these reasons about using 3 booleans.

================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:183
+    break;
+  case BO_NE: // Noop.
+    break;
----------------
ymandel wrote:
> why not? its a test, so not necessary, just curious to know why you're leaving this out.
This was my thought process: There is nothing that we can say about `b` if `a` is positive. Because `b` can still be either negative, positive, or even zero. Similar reasoning goes to the case when `a` is negative. 

However, we could have an implication if `a` is zero, namely that `b` is either positive or negative. But I think implications with `or` are not useful, they should be then rather expressed in the lattice. So, we should rather have a new boolean value like PosOrNeg, but I've found that too complicated for this demo.

================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:396
+
+// FIXME add this to testing support.
+template <typename NodeType, typename MatcherType>
----------------
ymandel wrote:
> It doesn't appear to be used.
Yes, good catch, it's been left here for some older version of the implementation. I removed it now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136668