[MLIR][LLVM] Extend argument/result attribute support.

[definelicht]

Use disallowlists to deselect attributes on arguments and return values that are not yet supported, but allow the rest to be present when inlining.

To achieve fast lookup in isLegalToInline the sets are cached on the inliner interface object during construction. By using sets of StringAttr this should just amount to a pointer lookup.

[gysit]

I think that works but I wonder what Alex or so says. I assume they probably don't like that the interface has state. But since we can recompute it when ever the interface is recreated it should be fine.

The solution with a thread_local static variable I proposed below may be nice as well (assuming it works :)). It initializes the set exactly once per thread. Not sure when the actual interface is instantiated.

[gysit]

A possible alternative may be to have a static function with a static and thread local member:

static bool isDisallowedArgumentAttr(NamedAttribute attr) {
  thread_local DenseSet<StringAttr> disallowedAttrs = {
    StringAttr::get(attr.getContext(), LLVM::LLVMDialect::getAlignAttrName()),
    ...
  };
  return disallowedAttrs.contains(attr.getName());
}

That would result in one copy of the set per thread if I understand correctly.

[definelicht]

Why thread_local and not just static?

[gysit]

I assume the initialization of a static is not thread safe and it may happen in multiple inliner threads at the same time?

[definelicht]

static should be at load time, I think? Not really an expert here, to be honest

[definelicht]

Reporting back: This does not seem to work; the variables are indeed only initialized once, but contains never finds anything, probably because they have been initialized with StringAttrs from a different context, and thus the pointer comparison fails. So I think this would only work by doing string comparisons. Hence the current solution seems better. But I understand the controversy... Unfortunately it seems to me like there's no way of doing this without keeping state on the specific dialect instantiation 🤕

[gysit]

static should be at load time, I think? Not really an expert here, to be honest

in C++ a static variable is initialized when the function is called for the first time AFAIK.

[definelicht]

in C++ a static variable is initialized when the function is called for the first time AFAIK.

Yes, apparently this is the case in C++11 and onwards. It's thread safe, so that's not a problem. The problem is that StringAttrs are tried to a specific context, and the context can change throughout execution, so the lookup does not work when a new object is instantiated with a different context, because it will not re-initialize the maps.

The current solution with having the maps on the object itself is the "correct" one as far as I can tell 🙂

[gysit]

Reporting back: This does not seem to work; the variables are indeed only initialized once, but contains never finds anything, probably because they have been initialized with StringAttrs from a different context, and thus the pointer comparison fails. So I think this would only work by doing string comparisons. Hence the current solution seems better. But I understand the controversy... Unfortunately it seems to me like there's no way of doing this without keeping state on the specific dialect instantiation 🤕

Ok that's a shame. We could probably refill the array once the context changes.

[gysit]

LGTM