Fix - CodeExtractor : Inherit Target Dependent Attributes from the parent function.

[rriddle]

Summary

When extracting a set of blocks make sure to inherit all of the target dependent attributes to make sure that the function will be valid for lowering. One example is the "target-features" attribute for x86, if the extracted region has functionality that relies on a specific feature it will fail to be lowered.
This also allows for extracted functions to be valid for inlining, at least back into the parent function, as the target attributes are tested when inlining for compatibility.

[majnemer]

/Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Utils/CodeExtractor.cpp

344 ↗

(On Diff #65201)

space after if

353 ↗

(On Diff #65201)

space after for

388 ↗

(On Diff #65201)

space before {

/Users/rriddle/Desktop/llvm/llvm/test/Transforms/CodeExtractor/2016-07-20-InheritAttributes.ll

33–34 ↗

(On Diff #65201)

This doesn't do anything because you never pipe into FileCheck.

[rriddle]

Updated formatting and fixed error in test case

[silvas]

The convention of putting the date in the test file name is no longer in fashion, so you can call the test file just InheritAttributes.ll

/Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Utils/CodeExtractor.cpp

393 ↗

(On Diff #65252)

Can you split this into a separate patch with its own test case?

/Users/rriddle/Desktop/llvm/llvm/test/Transforms/CodeExtractor/2016-07-20-InheritAttributes.ll

1 ↗

(On Diff #65252)

You should also add a RUN line that pipes to FileCheck and actually checks for the expected result (e.g. specific attributes on the extracted function).

[silvas]

/Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Utils/CodeExtractor.cpp

352 ↗

(On Diff #65252)

Can you add a FIXME here that there may be target-dependent attributes that can't be copied?

[rriddle]

Updated after Sean's comments

[silvas]

LGTM with a nit

test/Transforms/CodeExtractor/InheritTargetAttributes.ll

3 ↗

(On Diff #66022)

Can you mention what explicitly goes wrong if we don't inherit them?

[rriddle]

Fixed test description

[silvas]

Do you need me to commit this for you?

[silvas]

Should be committed in r277315, thanks!

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

Function *CodeExtractor::constructFunction(const ValueSet &inputs,
   // Create the new function
   Function *newFunction = Function::Create(funcType,
                                            GlobalValue::InternalLinkage,
                                            oldFunction->getName() + "_" +
                                            header->getName(), M);
   // 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.
+  //  (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.
+  AttributeSet OldFnAttrs = oldFunction->getAttributes().getFnAttributes();
+  AttrBuilder AB(OldFnAttrs, AttributeSet::FunctionIndex);
+  for (auto Attr : AB.td_attrs())
+    newFunction->addFnAttr(Attr.first, Attr.second);
+
   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/InheritTargetAttributes.ll

+; RUN: opt < %s -partial-inliner | llc -filetype=null
+; RUN: opt < %s -partial-inliner -S | FileCheck %s
+; This testcase checks to see if CodeExtractor properly inherits
+;   target specific attributes for the extracted function. This can
+;   cause certain instructions that depend on the attributes to not
+;   be lowered. Like in this test where we try to 'select' the blendvps
+;   intrinsic on x86 that requires the +sse4.1 target feature.
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind readnone
+declare <4 x float> @llvm.x86.sse41.blendvps(<4 x float>, <4 x float>, <4 x float>) #0
+
+; Function Attrs: nounwind uwtable
+define <4 x float> @inlinedFunc(i1, <4 x float>, <4 x float>, <4 x float>) #1 {
+entry:
+  br i1 %0, label %if.then, label %return
+if.then:
+; Target intrinsic that requires sse4.1
+  %target.call = call <4 x float> @llvm.x86.sse41.blendvps(<4 x float> %1, <4 x float> %2, <4 x float> %3)
+  br label %return
+return:             ; preds = %entry
+  %retval = phi <4 x float> [ zeroinitializer, %entry ], [ %target.call, %if.then ]
+  ret <4 x float> %retval
+}
+
+; Function Attrs: nounwind uwtable
+define <4 x float> @dummyCaller(i1, <4 x float>, <4 x float>, <4 x float>) #1 {
+entry:
+  %val = call <4 x float> @inlinedFunc(i1 %0, <4 x float> %1, <4 x float> %2, <4 x float> %3)
+  ret <4 x float> %val
+}
+
+
+attributes #0 = { nounwind readnone }
+attributes #1 = { nounwind uwtable "target-cpu"="x86-64" "target-features"="+sse4.1" }
+
+; CHECK: define {{.*}} @inlinedFunc.1_if.then{{.*}} [[COUNT1:#[0-9]+]]
+; CHECK: [[COUNT1]] = { {{.*}} "target-cpu"="x86-64" "target-features"="+sse4.1" }