[PATCH] D62687: [Attributor] Deduce "nofree" function attribute

Johannes Doerfert via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu May 30 17:27:28 PDT 2019


jdoerfert added a comment.

In D62687#1523832 <https://reviews.llvm.org/D62687#1523832>, @uenoku wrote:

> Sorry, I was quite mistaken about Attributor. I removed TLI and followed comments.


You misunderstood, there is absolutely no need to apologize. I like the patch and it is totally normal that I have comments now.
This is the first patch to a new framework and I have a lot of thoughts in my head how the code should look like/interact with the rest.

The patch is almost ready as far as I can tell, I added some (mostly minor) comments though.

Did you actually run the pass and try it on the test case you have? The changes to the test case should be part of this patch (pretend the test cases are already in tree).



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:716
+class AANoFreeFunction final : public AbstractAttribute, BooleanState {
+public:
+  /// See AbstractAttribute::AbstractAttribute(...).
----------------
If you make it a struct you don't need the `public` anymore.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:731
+  }
+  virtual const std::string getAsStr() const override {
+    return getAssumed() ? "nofree" : "may-free";
----------------
Comment and new line pls.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:751
+  bool isAssumedNoFree() const { return getAssumed(); }
+  bool isKnownNoFree() const { return getKnown(); }
+
----------------
Comments please.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:769
+    for (Instruction *I : OpcodeInstMap[Opcode]) {
+      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
+        switch (II->getIntrinsicID()) {
----------------
Do we have any intrinsic that might free memory? I would need to double check but I don't think we have one.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:800
+      if ((!NoFreeAA || !NoFreeAA->isAssumedNoFree()) &&
+          !ICS.hasFnAttr("nofree")) {
+        indicatePessimisticFixpoint();
----------------
My other two comments were confusing and I should have explained this better.

The idea (I have) is:
Information should be determined through other attributes if possible. That way we do not need to look at the IR in different places and also ask if an attribute exist and what the status is. That would mean do not look at the IR attributes (through the CallSite). I also confused you with the `isAssumedNoFree` as it seems. I did want that in addition to the two checks you had, so `!NoFreeAA`, `!NoFreeAA->getState().hasValidState()`, and ` !NoFreeAA->isAssumedNoFree()`, even though the last two are equivalent it makes it easier to read and consistent with more complex attributes.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:971
     switch (I.getOpcode()) {
     default:
       break;
----------------
Please add an `assert` here to make sure we did not miss any CallBase instruction opcode.


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

https://reviews.llvm.org/D62687





More information about the llvm-commits mailing list