[Attributor] Introduce Attribute seed allow list.

[kuter]

Summary

This patch introduces a command line option for allowing only selected attributes to be seeded.

[kuter]

Replace ternary expression with early return

[sstefan1]

Can you add a test using this option?

llvm/lib/Transforms/IPO/Attributor.cpp

1465

would it make sense to make this always check lower case names, to avoid mistakes?

[kuter]

Introduce test for the command line option

[kuter]

llvm/lib/Transforms/IPO/Attributor.cpp

1465

Since the name is usually the name of the class that defines the attribute,
I think it makes for sense the use the name as is.

[uenoku]

Either is fine but I think it is more natural to forbid an empty list.

[kuter]

In D83185#2134275, @uenoku wrote:

Either is fine but I think it is more natural to forbid an empty list.

Do you mean returning a error if a empty --attributor-seed-allow-list option is present ?
Currently the size of list is being used to tell if a list is present or not.
I think I can use getNumOccurrences() to replace this behaviour .

[uenoku]

In D83185#2134395, @kuter wrote:

In D83185#2134275, @uenoku wrote:

Either is fine but I think it is more natural to forbid an empty list.

Do you mean returning a error if a empty --attributor-seed-allow-list option is present ?
Currently the size of list is being used to tell if a list is present or not.
I think I can use getNumOccurrences() to replace this behaviour .

Yes, I think replacing ZeroOrMore with OneOrMore is enoguh

[kuter]

In D83185#2134444, @uenoku wrote:

In D83185#2134395, @kuter wrote:

In D83185#2134275, @uenoku wrote:

Either is fine but I think it is more natural to forbid an empty list.

Do you mean returning a error if a empty --attributor-seed-allow-list option is present ?
Currently the size of list is being used to tell if a list is present or not.
I think I can use getNumOccurrences() to replace this behaviour .

Yes, I think replacing ZeroOrMore with OneOrMore is enoguh

I think cl::OneOrMore causes a error to be generated if the option is not specified.
That would make it mandatory to have a atleast one --attributor-seed-allow-list parameter for every invocation of
opt

https://llvm.org/docs/CommandLine.html

The cl::OneOrMore modifier indicates that the option must be specified at least one time.

[uenoku]

In D83185#2134452, @kuter wrote:

In D83185#2134444, @uenoku wrote:

In D83185#2134395, @kuter wrote:

In D83185#2134275, @uenoku wrote:

Either is fine but I think it is more natural to forbid an empty list.

Do you mean returning a error if a empty --attributor-seed-allow-list option is present ?
Currently the size of list is being used to tell if a list is present or not.
I think I can use getNumOccurrences() to replace this behaviour .

Yes, I think replacing ZeroOrMore with OneOrMore is enoguh

I think cl::OneOrMore causes a error to be generated if the option is not specified.
That would make it mandatory to have a atleast one --attributor-seed-allow-list parameter for every invocation of
opt

https://llvm.org/docs/CommandLine.html

The cl::OneOrMore modifier indicates that the option must be specified at least one time.

Yeah, I might be wrong. Sorry for confusion.

[jdoerfert]

As discussed, we can check for the validity of the list in the constructor. Two more comments below.

llvm/include/llvm/Transforms/IPO/Attributor.h

1432

Use more words to describe this please.

Nit: Maybe initialize it directly here.

llvm/test/Transforms/Attributor/allow_list.ll

38

Get rid of all lines we don't need, e.g. the last 5.

[kuter]

1. Better comments.
2. Remove unneeded lines from the test.

I investigated the situation with the argument option that this patch introduces.
LLVM CommandLine library already does not allow a empty list but what happens is when you
pass a empty list for a execution like this opt --attribute-seed-allow-list
CommandLine automatically displays an error and terminates execution.
But when there are other arguments after the option the then that option becomes a member of the list.

It would have been great if we had a way of telling whether a name is valid or not.
Perhaps we could have a list of valid attribute names.

[jdoerfert]

It would have been great if we had a way of telling whether a name is valid or not. Perhaps we could have a list of valid attribute names.

Let's not worry about that too much now. It seems an empty list doesn't make sense and when people pass garbage they'll get something.

One nit below and one idea but otherwise LGTM.

We could set the seeding to false if the list is empty to avoid looking up if it is empty all the time. Feel free to ignore.

llvm/include/llvm/Transforms/IPO/Attributor.h

1432

Nit: called. \see ...

llvm/include/llvm/Transforms/IPO/Attributor.h

if (AAType *AAPtr = lookupAAFor<AAType>(IRP, QueryingAA, TrackDependence)) {
       if (ForceUpdate)
         updateAA(*AAPtr);
       return *AAPtr;
     }
 
     // No matching attribute found, create one.
     // Use the static create method.
     auto &AA = AAType::createForPosition(IRP, *this);
+
+    // If we are currenty seeding attributes, enforce seeding rules.
+    if (SeedingPeriod && !shouldSeedAttribute(AA)) {
+      AA.getState().indicatePessimisticFixpoint();
+      return AA;
+    }
+
     registerAA(AA);
 
     // For now we ignore naked and optnone functions.
     bool Invalidate = Allowed && !Allowed->count(&AAType::ID);
     const Function *FnScope = IRP.getAnchorScope();
     if (FnScope)
       Invalidate |= FnScope->hasFnAttribute(Attribute::Naked) ||
                     FnScope->hasFnAttribute(Attribute::OptimizeNone);
const AAType &getOrCreateAAFor(const IRPosition &IRP,
     // We can initialize (=look at) code outside the current function set but
     // not call update because that would again spawn new abstract attributes in
     // potentially unconnected code regions (=SCCs).
     if (FnScope && !Functions.count(const_cast<Function *>(FnScope))) {
       AA.getState().indicatePessimisticFixpoint();
       return AA;
     }
 
+    // Allow seeded attributes to declare dependencies.
+    // Remember the seeding state.
+    bool OldSeedingPeriod = SeedingPeriod;
+    SeedingPeriod = false;
+
     updateAA(AA);
 
+    SeedingPeriod = OldSeedingPeriod;
+
     if (TrackDependence && AA.getState().isValidState())
       recordDependence(AA, const_cast<AbstractAttribute &>(*QueryingAA),
                        DepClass);
     return AA;
   }
 
   /// Return the attribute of \p AAType for \p IRP if existing. This also allows
   /// non-AA users lookup.
bool checkForAllCallSites(function_ref<bool(AbstractCallSite)> Pred,
                             bool &AllCallSitesKnown);
 
   /// Apply all requested function signature rewrites
   /// (\see registerFunctionSignatureRewrite) and return Changed if the module
   /// was altered.
   ChangeStatus
   rewriteFunctionSignatures(SmallPtrSetImpl<Function *> &ModifiedFns);
 
+  /// Check if the Attribute \p AA should be seeded.
+  /// See getOrCreateAAFor.
+  bool shouldSeedAttribute(AbstractAttribute &AA);
+
   /// The set of all abstract attributes.
   ///{
   using AAVector = SmallVector<AbstractAttribute *, 64>;
   AAVector AllAbstractAttributes;
   ///}
 
   /// A nested map to lookup abstract attributes based on the argument position
   /// on the outer level, and the addresses of the static member (AAType::ID) on
private:
   DenseMap<Use *, Value *> ToBeChangedUses;
 
   /// Instructions we replace with `unreachable` insts after manifest is done.
   SmallDenseSet<WeakVH, 16> ToBeChangedToUnreachableInsts;
 
   /// Invoke instructions with at least a single dead successor block.
   SmallVector<WeakVH, 16> InvokeWithDeadSuccessor;
 
+  /// Wheather attributes are being `seeded`, always false after ::run function
+  /// gets called see getOrCreateAAFor.
+  bool SeedingPeriod = true;
+
   /// Functions, blocks, and instructions we delete after manifest is done.
   ///
   ///{
   SmallPtrSet<Function *, 8> ToBeDeletedFunctions;
   SmallPtrSet<BasicBlock *, 8> ToBeDeletedBlocks;
   SmallDenseSet<WeakVH, 8> ToBeDeletedInsts;
   ///}
 };

llvm/lib/Transforms/IPO/Attributor.cpp

static cl::opt<bool> EnableHeapToStack("enable-heap-to-stack-conversion",
                                        cl::init(true), cl::Hidden);
 
 static cl::opt<bool>
     AllowShallowWrappers("attributor-allow-shallow-wrappers", cl::Hidden,
                          cl::desc("Allow the Attributor to create shallow "
                                   "wrappers for non-exact definitions."),
                          cl::init(false));
 
+static cl::list<std::string>
+    SeedAllowList("attributor-seed-allow-list", cl::Hidden,
+                  cl::desc("Comma seperated list of attrbute names that are "
+                           "allowed to be seeded."),
+                  cl::ZeroOrMore, cl::CommaSeparated);
+
 /// Logic operators for the change status enum class.
 ///
 ///{
 ChangeStatus llvm::operator|(ChangeStatus l, ChangeStatus r) {
   return l == ChangeStatus::CHANGED ? l : r;
 }
 ChangeStatus llvm::operator&(ChangeStatus l, ChangeStatus r) {
   return l == ChangeStatus::UNCHANGED ? l : r;
for (Function *F : Functions) {
     assert(!verifyFunction(*F, &errs()) && "Module verification failed!");
   }
 #endif
 
   return ManifestChange;
 }
 
 ChangeStatus Attributor::run() {
+  SeedingPeriod = false;
   runTillFixpoint();
   ChangeStatus ManifestChange = manifestAttributes();
   ChangeStatus CleanupChange = cleanupIR();
   return ManifestChange | CleanupChange;
 }
 
 ChangeStatus Attributor::updateAA(AbstractAttribute &AA) {
   // Use a new dependence vector for this update.
bool Attributor::registerFunctionSignatureRewrite(
   // Remember the replacement.
   ARI.reset(new ArgumentReplacementInfo(*this, Arg, ReplacementTypes,
                                         std::move(CalleeRepairCB),
                                         std::move(ACSRepairCB)));
 
   return true;
 }
 
+bool Attributor::shouldSeedAttribute(AbstractAttribute &AA) {
+  if (SeedAllowList.size() == 0)
+    return true;
+  return std::count(SeedAllowList.begin(), SeedAllowList.end(), AA.getName());
+}
+
 ChangeStatus Attributor::rewriteFunctionSignatures(
     SmallPtrSetImpl<Function *> &ModifiedFns) {
   ChangeStatus Changed = ChangeStatus::UNCHANGED;
 
   for (auto &It : ArgumentReplacementMap) {
     Function *OldFn = It.getFirst();
 
     // Deleted functions do not require rewrites.

llvm/test/Transforms/Attributor/allow_list.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
+; RUN: opt -S -passes=attributor --attributor-seed-allow-list asd < %s | FileCheck %s --check-prefixes=CHECK_DISABLED
+; RUN: opt -S -passes=attributor --attributor-seed-allow-list AAValueSimplify < %s | FileCheck %s --check-prefixes=CHECK_ENABLED
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+; Function Attrs: nounwind uwtable
+define internal i32 @range_test(i32 %a) #0 {
+; CHECK_DISABLED-LABEL: define {{[^@]+}}@range_test
+; CHECK_DISABLED-SAME: (i32 [[A:%.*]])
+; CHECK_DISABLED-NEXT:    [[TMP1:%.*]] = icmp sgt i32 [[A]], 100
+; CHECK_DISABLED-NEXT:    [[TMP2:%.*]] = zext i1 [[TMP1]] to i32
+; CHECK_DISABLED-NEXT:    ret i32 [[TMP2]]
+;
+  %1 = icmp sgt i32 %a, 100
+  %2 = zext i1 %1 to i32
+  ret i32 %2
+}
+
+; Function Attrs: nounwind uwtable
+define i32 @range_use() #0 {
+; CHECK_DISABLED-LABEL: define {{[^@]+}}@range_use()
+; CHECK_DISABLED-NEXT:    [[TMP1:%.*]] = call i32 @range_test(i32 123)
+; CHECK_DISABLED-NEXT:    ret i32 [[TMP1]]
+;
+; CHECK_ENABLED-LABEL: define {{[^@]+}}@range_use()
+; CHECK_ENABLED-NEXT:    ret i32 1
+;
+  %1 = call i32 @range_test(i32 123)
+  ret i32 %1
+}
+
+attributes #0 = { nounwind uwtable noinline }
 No newline at end of file