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

[Florian Hahn via Phabricator via llvm-commits]

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,
----------------
fhahn wrote:
> 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.
Any more thoughts on  the above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152081