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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Some high-level comments. I need to go over the logic later on.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3071
+//                        cl::init(7));
+static const unsigned MaxPotentialValues = 7;
+
----------------
What you can do is:
1) Put the static value as non-const into the PotentialValueState class below.
2) put the command line option into the cpp file.
3) use the static location as the "location" of the command line option, that is the place the value is put. Search the code for `cl::location` for examples.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3185
+  /// Set representing assumed set of potential values.
+  StateTy Assumed;
+};
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:4400
+                                             conflictAssumedConstantInt);
+    return !conflictAssumedConstantInt && !failAll;
   }
----------------
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!


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:4708
+  }
+
   void trackStatistics() const override {
----------------
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?


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7118
+    OS << "} >";
+    return OS.str();
+  }
----------------
This looks like the abstract state print code I saw before. Can we reuse it to print the state here?


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

https://reviews.llvm.org/D80991