[CodeExtractor] Use subset of function attributes for extracted function.

[fhahn]

Summary

In addition to target-dependent attributes, we can also preserve a
white-listed subset of target independent function attributes. The white-list
excludes problematic attributes, most prominently:

• attributes related to memory accesses, as alloca instructions could be moved in/out of the extracted block

• control-flow dependent attributes, like no_return or thunk, as the relerelevant instructions might or might not get extracted.

Thanks @efriedma and @aemerson for providing a set of attributes that cannot be
propagated.

[fhahn]

Post holiday ping. Happy new year! This updates CodeExtractor to propagate the function attributes for the original function to the extracted function (and the call to the extracted function). As the extracted function is a subset of the original function, I think that should be safe and desired (e.g. correctly preserving fastmath flags or ARM thumbness)

[aemerson]

In D41334#965887, @fhahn wrote:

Post holiday ping. Happy new year! This updates CodeExtractor to propagate the function attributes for the original function to the extracted function (and the call to the extracted function). As the extracted function is a subset of the original function, I think that should be safe and desired (e.g. correctly preserving fastmath flags or ARM thumbness)

What about the noreturn attribute? Obviously the extracted function must include a return so it seems this would need to be dropped.

[fhahn]

In D41334#965905, @aemerson wrote:

What about the noreturn attribute? Obviously the extracted function must include a return so it seems this would need to be dropped.

Right, the part that does not return may not get extracted and would be executed after the call to the extracted function. So we should drop it, thanks! Are there any additional attributes we should be dropping?

[fhahn]

I suppose we should also drop thunk, as the tailcall could stay in the non-extracted part and the extracted part may not end in a tailcall?

[efriedma]

You also can't copy readnone/readonly/writeonly/argmemonly/inaccessiblememonly/inaccessiblemem_or_argmemonly: the extracted code could read/modify an alloca allocated in the parent function.

allocsize also can't be copied for obvious reasons. Same for returns_twice.

patchable-function doesn't make sense to copy; not sure it would do any harm, but doesn't do anything useful. Same with alignstack.

Not sure about convergent.

And of course, there's a whole bunch of target-specific attributes, but hopefully those are all safe to copy.

[fhahn]

Thanks Eli & Amara. I've substantially changed the code to use a subset of whitelisted function attributes from the original function for the extracted function.

[efriedma]

lib/Transforms/Utils/CodeExtractor.cpp

636 ↗

(On Diff #128614)

Could we explicitly list out all the attributes here, so the compiler will warn if a new attribute gets added?

644 ↗

(On Diff #128614)

Not sure about propagating JumpTable and Naked. (Granted, we probably shouldn't be extracting code from Naked functions anyway.)

[fhahn]

Explicitly state unsafe attributes and don't propagate Naked and JumpTable. Also adding Sean, as he added the target-dependent propagation I think.

[rriddle]

I was actually responsible for the target dependent attrs patch, Sean was nice enough to commit for me at the time.

This looks fine.

Would have looked sooner but I've been in Japan for a while.

[efriedma]

LGTM

llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp

Function *CodeExtractor::constructFunction(const ValueSet &inputs,
   // If the old function is no-throw, so is the new one.
   if (oldFunction->doesNotThrow())
     newFunction->setDoesNotThrow();
 
   // Inherit the uwtable attribute if we need to.
   if (oldFunction->hasUWTable())
     newFunction->setHasUWTable();
 
-  // Inherit all of the target dependent attributes.
+  // Inherit all of the target dependent attributes and white-listed
+  // target independent attributes.
   //  (e.g. If the extracted region contains a call to an x86.sse
   //  instruction we need to make sure that the extracted region has the
   //  "target-features" attribute allowing it to be lowered.
   // FIXME: This should be changed to check to see if a specific
   //           attribute can not be inherited.
-  AttrBuilder AB(oldFunction->getAttributes().getFnAttributes());
-  for (const auto &Attr : AB.td_attrs())
-    newFunction->addFnAttr(Attr.first, Attr.second);
+  for (const auto &Attr : oldFunction->getAttributes().getFnAttributes()) {
+    if (Attr.isStringAttribute()) {
+      if (Attr.getKindAsString() == "thunk")
+        continue;
+    } else
+      switch (Attr.getKindAsEnum()) {
+      // Those attributes cannot be propagated safely. Explicitly list them
+      // here so we get a warning if new attributes are added. This list also
+      // includes non-function attributes.
+      case Attribute::Alignment:
+      case Attribute::AllocSize:
+      case Attribute::ArgMemOnly:
+      case Attribute::Builtin:
+      case Attribute::ByVal:
+      case Attribute::Convergent:
+      case Attribute::Dereferenceable:
+      case Attribute::DereferenceableOrNull:
+      case Attribute::InAlloca:
+      case Attribute::InReg:
+      case Attribute::InaccessibleMemOnly:
+      case Attribute::InaccessibleMemOrArgMemOnly:
+      case Attribute::JumpTable:
+      case Attribute::Naked:
+      case Attribute::Nest:
+      case Attribute::NoAlias:
+      case Attribute::NoBuiltin:
+      case Attribute::NoCapture:
+      case Attribute::NoReturn:
+      case Attribute::None:
+      case Attribute::NonNull:
+      case Attribute::ReadNone:
+      case Attribute::ReadOnly:
+      case Attribute::Returned:
+      case Attribute::ReturnsTwice:
+      case Attribute::SExt:
+      case Attribute::Speculatable:
+      case Attribute::StackAlignment:
+      case Attribute::StructRet:
+      case Attribute::SwiftError:
+      case Attribute::SwiftSelf:
+      case Attribute::WriteOnly:
+      case Attribute::ZExt:
+      case Attribute::EndAttrKinds:
+        continue;
+      // Those attributes should be safe to propagate to the extracted function.
+      case Attribute::AlwaysInline:
+      case Attribute::Cold:
+      case Attribute::NoRecurse:
+      case Attribute::InlineHint:
+      case Attribute::MinSize:
+      case Attribute::NoDuplicate:
+      case Attribute::NoImplicitFloat:
+      case Attribute::NoInline:
+      case Attribute::NonLazyBind:
+      case Attribute::NoRedZone:
+      case Attribute::NoUnwind:
+      case Attribute::OptimizeNone:
+      case Attribute::OptimizeForSize:
+      case Attribute::SafeStack:
+      case Attribute::SanitizeAddress:
+      case Attribute::SanitizeMemory:
+      case Attribute::SanitizeThread:
+      case Attribute::SanitizeHWAddress:
+      case Attribute::StackProtect:
+      case Attribute::StackProtectReq:
+      case Attribute::StackProtectStrong:
+      case Attribute::StrictFP:
+      case Attribute::UWTable:
+        break;
+      }
 
+    newFunction->addFnAttr(Attr);
+  }
   newFunction->getBasicBlockList().push_back(newRootNode);
 
   // Create an iterator to name all of the arguments we inserted.
   Function::arg_iterator AI = newFunction->arg_begin();
 
   // Rewrite all users of the inputs in the extracted region to use the
   // arguments (or appropriate addressing into struct) instead.
   for (unsigned i = 0, e = inputs.size(); i != e; ++i) {

llvm/trunk/test/Transforms/CodeExtractor/PartialInlineAttributes.ll

+; RUN: opt < %s -S -partial-inliner -skip-partial-inlining-cost-analysis=true | FileCheck %s
+
+
+define i32 @callee_most(i32 %v) unnamed_addr  #0 #1 {
+entry:
+  %cmp = icmp sgt i32 %v, 2000
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:
+  br label %if.then2
+
+if.then2:
+  %sub = sub i32 %v, 10
+  br label %if.end
+
+if.end:
+  %v2 = phi i32 [ %v, %entry ], [ %sub, %if.then2 ]
+  %add = add nsw i32 %v2, 200
+  ret i32 %add
+}
+
+define i32 @callee_noinline(i32 %v) optnone noinline {
+entry:
+  %cmp = icmp sgt i32 %v, 2000
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:
+  br label %if.then2
+
+if.then2:
+  %sub = sub i32 %v, 10
+  br label %if.end
+
+if.end:
+  %v2 = phi i32 [ %v, %entry ], [ %sub, %if.then2 ]
+  %add = add nsw i32 %v2, 200
+  ret i32 %add
+}
+
+define i32 @callee_writeonly(i32 %v) writeonly {
+entry:
+  %cmp = icmp sgt i32 %v, 2000
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:
+  br label %if.then2
+
+if.then2:
+  %sub = sub i32 %v, 10
+  br label %if.end
+
+if.end:
+  %v2 = phi i32 [ %v, %entry ], [ %sub, %if.then2 ]
+  %add = add nsw i32 %v2, 200
+  ret i32 %add
+}
+; CHECK-LABEL: @caller
+; CHECK: call void @callee_most.2_if.then(i32 %v
+; CHECK: call i32 @callee_noinline(i32 %v)
+; CHECK: call void @callee_writeonly.1_if.then(i32 %v
+define i32 @caller(i32 %v) {
+entry:
+  %c1 = call i32 @callee_most(i32 %v)
+  %c2 = call i32 @callee_noinline(i32 %v)
+  %c3 = call i32 @callee_writeonly(i32 %v)
+  ret i32 %c3
+}
+
+; CHECK: define internal void @callee_writeonly.1_if.then(i32 %v, i32* %sub.out) { 
+; CHECK: define internal void @callee_most.2_if.then(i32 %v, i32* %sub.out)  [[FN_ATTRS:#[0-9]+]]
+
+; attributes to preserve
+attributes #0 = {
+  inlinehint minsize noduplicate noimplicitfloat norecurse noredzone nounwind
+  nonlazybind optsize safestack sanitize_address sanitize_hwaddress sanitize_memory
+  sanitize_thread ssp sspreq sspstrong strictfp uwtable "foo"="bar"
+  "patchable-function"="prologue-short-redirect" "probe-stack"="_foo_guard" "stack-probe-size"="4096" }
+
+; CHECK: attributes [[FN_ATTRS]] = { inlinehint minsize noduplicate noimplicitfloat norecurse noredzone nounwind nonlazybind optsize safestack sanitize_address sanitize_hwaddress sanitize_memory sanitize_thread ssp sspreq sspstrong strictfp uwtable "foo"="bar" "patchable-function"="prologue-short-redirect" "probe-stack"="_foo_guard" "stack-probe-size"="4096" }
+
+; attributes to drop
+attributes #1 = {
+  alignstack=16 convergent inaccessiblememonly inaccessiblemem_or_argmemonly naked
+  noreturn readonly argmemonly returns_twice speculatable "thunk"
+}