[PATCH] D109033: [InlineCost] Introduce attributes to override InlineCost for inliner testing

[Daniil Suchkov via Phabricator via llvm-commits]

DaniilSuchkov added inline comments.


================
Comment at: llvm/include/llvm/IR/InstrTypes.h:2282
 
+  template <typename AttrKind> Attribute getFnAttrImpl(AttrKind Kind) const {
+    Attribute CallAttr = Attrs.getFnAttr(Kind);
----------------
aeubanks wrote:
> I'd rather not change the semantics of `getFnAttr`, can this be in InlineCost instead?
This change makes getFnAttr's behavior similar to the one of hasFnAttr and hasRetAttr. Right now you can have a case when `CB.getFnAttr(Attr).isValid() != CB.hasFnAttr(Attr)` is true, which doesn't seem right, so I think that fixing it is better than creating a helper function that works around it.

However, I would agree that
a) what I described is a separate issue, definitely out of scope of this patch;
b) this change makes it way more likely that this patch will break something and will be reverted because of that
So a better decision is to add a workaround here, as you suggested, and then (maybe) fix this API inconsistency later in a separate patch.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:138
 
+template <typename IntTy> Optional<IntTy> stringAttrAsInt(Attribute Attr) {
+  static_assert(std::is_integral<IntTy>::value, "Expected integer type!");
----------------
aeubanks wrote:
> making this a template seems unnecessary, just use `int`? that's what all the callers are using anyway
Fair point.


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

https://reviews.llvm.org/D109033