[PATCH] D80991: [WIP][Attributor] AAPotentialValues Attribute

[Shinji Okumura via Phabricator via llvm-commits]

okura marked 3 inline comments as done.
okura added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3185
+  /// Set representing assumed set of potential values.
+  StateTy Assumed;
+};
----------------
jdoerfert wrote:
> I'm not sure if we really want/need to track the known set here. Maybe I just don't understand what we currently track in it. Feel free to explain :)
> If it is "just" to match the design of other states, don't worry. Take the full set as the worst state and remove the known stuff.
`Known.IsFull` is always true in the current implementation, But when the bit width of an value is 1 or 2,  we can initialize known set as actual full-set (`isFull` is false).
By this, the fixpoint state can be reached faster and we may be able to reduce the number of iterations. That's why I left known in here. Is this a bad idea?


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:4400
+                                             conflictAssumedConstantInt);
+    return !conflictAssumedConstantInt && !failAll;
   }
----------------
jdoerfert wrote:
> Could you explain how we could end up with conflicting values? I was expecting we define an order here, ask the first pass, if that fails the second.
> 
> The design to put it all in the template helper is very nice!
I noticed that even if the current assumptions contradict, the assumption must be broken eventually. So I'll fix this.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:4708
+  }
+
   void trackStatistics() const override {
----------------
jdoerfert wrote:
> This looks like something we need to split off. If you add this stand-alone, do we see an effect in the tests, or could you create a test for this?
When I modified failed tests, I figured out the cause of unexpected simplification is here. The problem is not directly related to `AAPotentialValues`, I will split.
(actually related to  `AAConstantRange`). I have a test in which inappropriate simplification occurs. I will make a new patch.


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

https://reviews.llvm.org/D80991