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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:2960
+static const APInt EmptyKey = APInt::getAllOnesValue(100);
+static const APInt TombstoneKey = APInt::getAllOnesValue(101);
+
----------------
What is 100 and 101 here? I think the declarations should go into the class below so they are not in global namespace.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:2974
+  }
+};
+
----------------
Unsure if you need/want to inherit from the `void*` specialization.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:2976
+
+struct PotentialValueSet {
+  using SetTy = DenseSet<APInt>;
----------------
You might want to make this a template class parametrized with `MemberTy`. Below you can specialize it `struct PotentialConstantValueSet : public PotentialValueSet<APInt> {};`


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3007
+      Set.insert(e);
+    }
+  }
----------------
Style: replace `auto &` with `const APInt &` and `e` with `C` maybe. Also no braces.
Check if the set doesn't have a union member already.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3027
+      Set.erase(e);
+    }
+  }
----------------
This doesn't actually do intersect, I think. If there is no intersect member collect all new value not the ones to be erased. That way it should work more easily. See also set comments above.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3031
+  bool isFull;
+  SetTy Set;
+};
----------------
Add documentation here. Also explain in the class comment or here what it means for a set to be full.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3049
+    return !Assumed.isFull && Assumed.Set.size() < 7;
+  }
+
----------------
7 needs to be replaced by a command line option


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3124
+
+  StateTy Known, Assumed;
+};
----------------
Do you really need the known state or would a boolean suffice to track if the state is fix and valid? If it's the latter, you can use a BooleanState member for that.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:3427
 
-
       // For now we do not try to "increase" dereferenceability due to negative
----------------
Formatting changes should be moved to an `[NFC]` commit before the patch.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:4422
+  }
+
   /// See AbstractAttribute::manifest(...).
----------------
Can we try to merge this function and the one above. Maybe use a template for the type for the queried AA, i mean `AAPotentialValues`.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:4514
+    }
+
     auto PredForCallSite = [&](AbstractCallSite ACS) {
----------------
We should not eagerly call these but instead try value simplify first. Maybe wrap the two functions above in a wrapper that is called below (line 4539) or merge the two functions above if that makes sense.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:4587
+      LLVM_DEBUG(dbgs() << *(SimplifiedAssociatedValue.getValue()) << "\n");
+    }
 
----------------
Same comment as last.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:4695
+                        << *(SimplifiedAssociatedValue.getValue()) << "\n");
 
     // If a candicate was found in this update, return CHANGED.
----------------
Same comment as last.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7171
+                      << getAssociatedValue() << "\n");
+  }
+
----------------
not needed.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7467
+                                           : ChangeStatus::CHANGED;
+    }
+
----------------
Put these large conditionals in separate helper functions please.


================
Comment at: llvm/test/Transforms/Attributor/potential.ll:24
+; IS__TUNIT____-LABEL: @potential_test1(
+; IS__TUNIT____-NEXT:    ret i1 false
+;
----------------
YaY! :)


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

https://reviews.llvm.org/D80991