[PATCH] D83283: [Attributor] AAPotentialValues Interface

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

The code is quite good and clean. Most comments are on the doc so that we can make it good and clean too and move to the full patch.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:94

----------------
We need to add a clearer comment here because it is still not clear what `isAtWorstState` means. Add sth like: "The worst state is when the set, theoretically, contains every possible value. That never happens naturally, we only force it". With that, the doc below will make sense.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:177

----------------
Nit: Maximum


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:188

----------------
"will grow while iteration". That doesn't make much sense. You can change it to "will grow monotonically as we find more potential values for this position". Or sth.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:191

----------------
Nit: Worst.
Good comment btw. Note that there are another 2 reasons that we might go to worst state. I would change it as follows:
"If the number of ... to a full set (the worse state)" -> "The set might (only) be forced to worst state, that is, to contain
every possible value (which effectively means that practically we know nothing about this IR position) in 3 cases:
1) We surpassed the \p MaxPotentialValues threshold.
2) We tried to initialize on a Value that we can't handle (e.g. an operator we don't currently handle)
3) During the iterations, this Value was affected (e.g. because of an operation) by a Value that is in
worst state.

I'm sure about the first 2, I have to think a little bit about whether the 3rd is good. What do you think for this
as a doc comment?


================
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:
> > 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?
> Yes, I tried to do it in that way, but I got many warnings like "DEBUG_TYPE is redefined" when I built.  So I gave up and redefine the struct here.
> Yes, I tried to do it in that way, but I got many warnings like "DEBUG_TYPE is redefined" when I built. So I gave up and redefine the struct here.

Hmm, ok I don't have a way to resolve that. Maybe someone else has some idea.


================
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]]
----------------
okura wrote:
> baziotis wrote:
> > 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).
> We cannot see the potential values in IR. Potential values are collected just for stronger value simplification for now.
> Unlike the original patch, `updateImpl` has not implemented yet in this patch. So expected value simplification was not achieved and we have different results.
You are correct, thanks for updating me. You know, we might be able to see the potential values on some tests from the ranges...
Anyway, those tests look good, we'll take a closer look to them on the other patch.


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

https://reviews.llvm.org/D83283