[PATCH] D152081: [Attributor] Add lightweight version for attribute deduction only. (WIP)

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3239
   if (!Attrs.hasAttribute(AK))
-    if (!AA::hasAssumedIRAttr<AK>(*this, nullptr, IRP, DepClassTy::NONE,
-                                  IsKnown))
-      getOrCreateAAFor<AAType>(IRP);
+    if (!Configuration.Allowed || Configuration.Allowed->count(&AAType::ID))
+      if (!AA::hasAssumedIRAttr<AK>(*this, nullptr, IRP, DepClassTy::NONE,
----------------
jdoerfert wrote:
> fhahn wrote:
> > I noticed that `checkAndQueryIRAttr` would add additional attributes if it's directly implied by the IR, which is not what we want at least to start with; I updated things here to check `Allowed` for now, to avoid inferring additional attributes that aren't part of the Allowed set.
> I don't see the point in this, honestly. This will only add known stuff, and we already don't have a 1:1 mapping, so it's unclear why we artificially prevent some known deductions that should not cost anything.
The main aim is to try to keep the inferred attributes as similar to legacy FunctionAttrs as possible, not necessarily for compile-time, but to also limit the functional differences. As @nikic mentioned earlier, the transition has the potential to expose a number of issues (incorrect inference of attributes, incorrect use of attributes in optimizations, UB in the user source code that now is exploited and causes mis-compiles).

To keep those issues manageable (and the pain to users to a minimum), IMO it would be desirable to only infer the attributes explicitly opted-in. Even though the attributes may be implied by the existing IR directly, it is still using code from the abstract attributes, so I think it would make sense to only apply it if the attribute is enabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152081