[PATCH] D83283: [Attributor] AAPotentialValues Interface

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

A nit comment is to please start comments with caps.

In general, this is good I think.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3056
+  static inline APInt getEmptyKey() {
+    APInt V(nullptr, 0);
+    V.U.VAL = 0;
----------------
okura wrote:
> baziotis wrote:
> > Is there an `APInt` constructor that takes a pointer and an `int` ? I think the only such constructor is a private one.
> Sorry for the delayed response. This struct is a friend struct of `APInt`. So Member functions can access the private functions of `APInt`. (c.f. https://llvm.org/doxygen/APInt_8h_source.html#l00099)
> On the other hand, the same struct is defined in "/lib/IR/LLVMContextImpl.h" and I want to include it but I failed to do that.
Yes, my bad, thank you. If you want to include that, I guess that the problem is that an `#include` "looks" in `llvm/include`. You want to go one dir back and to `lib`. So, `../lib/IR/...`. Have you tried that?


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3131
+  /// If this is true, the value of \p Set is meaningless.
+  bool isFull;
+
----------------
Please change this to `isAtWorstState` for the reasons discussed in the other review (in which though I was wrong to suggest `isTop` since it is ambiguous whether top is the worst or best state). And add a doc that e.g. the worst state is when the potential values are all the possible values and thus, we know nothing.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3190
+
+  /// Unite assumed set with the passed value.
+  void unionAssumed(const APInt &C) { Assumed.insert(C); }
----------------
Nit: "Unite" -> "Union" (it works as a verb as well). Same for other parts.


================
Comment at: llvm/test/Transforms/Attributor/potential.ll:59
+; IS__TUNIT____-SAME: (i32 [[C:%.*]])
+; IS__TUNIT____-NEXT:    [[CSRET1:%.*]] = call i32 @iszero2(i32 [[C]]) #0, !range !0
+; IS__TUNIT____-NEXT:    [[MINUSC:%.*]] = sub i32 0, [[C]]
----------------
I may be missing something here but how do you see the potential values in the output IR ? (Only) through ranges ?
I mean except for e.g. simplifying (e.g. that due to potential values, we deduce that the return value is in fact a constant, like in another test case).

Btw, we have different results that the test in the other diff right (this one: https://reviews.llvm.org/differential/changeset/?ref=2038557) ?
Why is that?

(I'll have to check the tests again).


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

https://reviews.llvm.org/D83283