[Inliner] Use whitelist instead of blacklist when checking function attribute compatibility and make the check stricter

[ahatanak]

Summary

This patch changes functionsHaveCompatibleAttribute to list the attributes that can be ignored rather than listing the attributes that should be checked (whitelist, instead of blacklist). I think this is better as we won't unintentionally inline a function that is incompatible with the caller when a new function attribute is added.

In my patch, I've whitelisted all function attributes listed in http://llvm.org/docs/LangRef.html#function-attributes except for these two:
alignstack(<n>)
noimplicitfloat

None of the string attributes have been added to the whitelist yet, so functions that have incompatible string attributes (for example, functions that have incompatible fast-math attributes) will not get inlined.

[ahatanak]

ping

[chandlerc]

I really don't like this. I really don't like the prior solution either.

Today, we have subtle, hard to diagnose correctness bugs. Bad bad bad.

With this we have subtle, hard to diagnose performance bugs combined with a bit of a maintenance nightmare to keep this up to date. Bad bad bad.

I also fear the performance bugs will in practice happen much more often. I'm seriously worried about the string attributes for example.

I think we need to back up and think of a better design for this. A *much* better design. Do you have any thoughts on how to do this in a more maintainable way? If not I'll try to write something else.

Naturally, if there are specific attributes that are casuing problems, let's get a targeted fix in place for those attributes.

[ahatanak]

I came up with a patch that addresses the concerns Chandler brought up. This is still a WIP patch, so I would like to hear feedback before I go too far down this route.

The updated patch takes a completely different approach to determining a function's inlinability. It uses table-gen to specify the attributes' enums along with their inlinability/compatibility information. With this new approach, whenever someone defines and uses a new attribute in the IR, the compatibility information has to be added too. Currently, there is only one field in class Attr in the .td file, which indicates whether the inliner should check the attribute's compatibility, but it should be easy to add other fields or rules if other passes need more information about the attribute. For example, I think most of the code in the verifier that looks at attributes in the IR can be auto-generated by table-gen.

The patch doesn't add any checks for the target-specific function attribute strings, but it can be done in a similar way to target-independent enum attributes. Functions are auto-generated by table-gen from .td files and are passed to the constructor of TargetIRAnalysis to enable checking compatibility of target-specific attributes. Also, we should be able to generate code that checks the validity of a string attribute added to the IR, which is something we currently lack. It would issue a warning or error if a string not recognized by the target was used to get an attribute from the attribute set.

[chandlerc]

Ok, finally back to this. Generally I do like the direction. Some more specific guidelines below.

include/llvm/Analysis/TargetTransformInfo.h

804–805 ↗

(On Diff #21626)

I would start off without trying to solve the problem for textual target attributes, and just take a conservative approach for them until we have a good use case, and can layer it on top.

include/llvm/IR/EnumAttributes.td

1 ↗

(On Diff #21626)

I really like this general approach. Could you start off the .td file with a comment explaining the syntax that should be used?

3 ↗

(On Diff #21626)

Rather than a single bit for compatibility checking, I'd like this to be a list of attributes to fall back on when merging.

I'm thinking of a structure like this.

def X : Attr<..., CompatSequence<[A, B, C]>>;
def Y : Attr<..., CompatSequence<[B, C]>>;

This would say that X is compatible with A, B, or C. So if we're inlining a Callee with X into A, B, or C, its fine. It would also mean that if we are inlining a callee with X into a caller Y, we could switch to attribute B instead as the first common attribute between them.

Thoughts?

[ahatanak]

Thank you for the review. A few comments inline.

include/llvm/Analysis/TargetTransformInfo.h

804–805 ↗

(On Diff #21626)

Yes, I think we can look at the target independent attributes first.

include/llvm/IR/EnumAttributes.td

3 ↗

(On Diff #21626)

I think we should use a list of attributes instead of a single bit if we want to check the compatibility between two different kinds of attributes. Do we need to do that for any of the target dependent or independent attributes we are currently using? I know we have to check attributes of the same kind in some cases (e.g., SanitizeAddress), but I'm not aware of any pair of different attribute kinds that can block inlining.

Also, if we want to make the list of attributes short, we should probably use lists of incompatible attributes instead of lists of compatible ones (or define table-gen constructs for both), as I think most attributes are always compatible with each other regardless of their values.

[echristo]

Hi Akira,

I think for right now since it's fairly trivial to come up with a testcase that will run into problems we should do a quick conservative patch to the inliner first that will enable some progress - i.e. we should check the target-cpu and target-features strings and if they don't match exactly go ahead and reject in the inliner. This is, obviously, much less than optimal, but will get us to an iterative solution.

Thoughts?

-eric

[ahatanak]

Yes, I that works for me. I've been working on rewriting the .td file, but
I can do that later.

There are other attributes that we need to check besides target-cpu and
target-features:

• Fast FP math attributes: I think we should reject inlining if the caller

has a fast fp-math attribute (e.g., unsafe-fp-math = true) but the callee
doesn't. Alternatively, we can change the caller's fp-math attribute (to
unsafe-fp-math = false). I know the long term solution would be to model
these attributes at the instruction-level, but until we make that change,
we should treat them as function attributes.

• NoImplicitFloat.
• Probably we should check some of the target-specific attributes too.

[ahatanak]

Sent a new patch here which changes inliner to check target-cpu and target-features:

http://reviews.llvm.org/D8984

[mkuper]

As a side note, I just ran into the - more or less - mirror image of this.

CodeExtractor currently doesn't copy any function attributes into the function it creates (except, for some reason, nounwind).
Copying all attributes, however, doesn't seem safe. For example, a readnone function can have a stack allocation and the pointer can be passed into the extracted function. On the other hand, we definitely want to copy target-cpu/target-features.

[ahatanak]

Update patch attached.

The following are the changes I made to the table-gen file:

• Added attributes classes which derive from the base class. This enables distinguishing enum attributes from string attributes and key-value attributes from attributes that don't have values.
• Added target-independent string attributes. Inliner uses these attributes to check caller-callee compatibility.
• Added two classes to describe function inlining rules.

I'm mainly interested in whether the table-gen syntax is easy to understand and is expressive enough to describe most of the inlining rules that are not so complex. Also, as I mentioned before, I think it's possible to make some improvements to it later so that it can be used to auto-generate code for other passes (e.g., IR verifier) or for documentation.

[ahatanak]

In D7802#155864, @mkuper wrote:

As a side note, I just ran into the - more or less - mirror image of this.

CodeExtractor currently doesn't copy any function attributes into the function it creates (except, for some reason, nounwind).
Copying all attributes, however, doesn't seem safe. For example, a readnone function can have a stack allocation and the pointer can be passed into the extracted function. On the other hand, we definitely want to copy target-cpu/target-features.

I can confirm CodeExtractor doesn't copy target-cpu and target-features. I haven't thought through the solution, but probably we can either fix CodeExtractor to recompute the attributes like readnone or remove them and let FunctionAttrs deduce them later.

[ahatanak]

Resurrecting the discussion on inliner's function attribute compatibility checking.

I rebased the patch and made the following changes:

• Changed the syntax of class "Attr" in Attributes.td. The new syntax allows specifying the c++ function that is called to check the compatibility between the attributes of the caller and callee.
• Updated cmake files.
• Add comments and renamed functions.

[ahatanak]

Rebase and add rules to merge caller's and callee's attributes to Attributes.td.

Merge rules enable modifying the caller's attribute set if the caller and callee have incompatible attribute sets, rather than blocking inlining. For example, if the callee has attribute "noimplicitfloat", but the caller doesn't, the merge rule defined in Attributes.td attaches "noimplicitfloat" to the caller.

[echristo]

I've got a few things here that are related and would like to take a look
if you wouldn't mind waiting while I take a look after Duncan's LGTM.

Thanks!

-eric

[echristo]

LGTM.

Thanks and sorry for the delay.

-eric

include/llvm/IR/Attributes.h

69 ↗

(On Diff #37320)

Can you separate this out into a separate patch? (Feel free to commit it as long as it's a nop change).

llvm/trunk/include/llvm/IR/Attributes.td

class MergeRule<string F> {
   // The name of the function called to merge the attributes of the caller and
   // callee. The function's signature must match
   // "void(Function&, const Function &)", where the first parameter is the
   // reference to the caller and the second parameter is the reference to the
   // callee.
   string MergeFunc = F;
 }
 
+def : MergeRule<"setAND<LessPreciseFPMADAttr>">;
+def : MergeRule<"setAND<NoInfsFPMathAttr>">;
+def : MergeRule<"setAND<NoNansFPMathAttr>">;
+def : MergeRule<"setAND<UnsafeFPMathAttr>">;
+def : MergeRule<"setOR<NoImplicitFloatAttr>">;
 def : MergeRule<"adjustCallerSSPLevel">;

llvm/trunk/test/Transforms/Inline/attributes.ll

 
 define i32 @test_target_features1(i32 %i) "target-features"="+sse4.2" {
   %1 = call i32 @test_target_features_callee1(i32 %i)
   ret i32 %1
 ; CHECK-LABEL: @test_target_features1(
 ; CHECK-NEXT: @test_target_features_callee1
 ; CHECK-NEXT: ret i32
 }
+
+define i32 @less-precise-fpmad_callee0(i32 %i) "less-precise-fpmad"="false" {
+  ret i32 %i
+; CHECK: @less-precise-fpmad_callee0(i32 %i) [[FPMAD_FALSE:#[0-9]+]] {
+; CHECK-NEXT: ret i32
+}
+
+define i32 @less-precise-fpmad_callee1(i32 %i) "less-precise-fpmad"="true" {
+  ret i32 %i
+; CHECK: @less-precise-fpmad_callee1(i32 %i) [[FPMAD_TRUE:#[0-9]+]] {
+; CHECK-NEXT: ret i32
+}
+
+define i32 @test_less-precise-fpmad0(i32 %i) "less-precise-fpmad"="false" {
+  %1 = call i32 @less-precise-fpmad_callee0(i32 %i)
+  ret i32 %1
+; CHECK: @test_less-precise-fpmad0(i32 %i) [[FPMAD_FALSE]] {
+; CHECK-NEXT: ret i32
+}
+
+define i32 @test_less-precise-fpmad1(i32 %i) "less-precise-fpmad"="false" {
+  %1 = call i32 @less-precise-fpmad_callee1(i32 %i)
+  ret i32 %1
+; CHECK: @test_less-precise-fpmad1(i32 %i) [[FPMAD_FALSE]] {
+; CHECK-NEXT: ret i32
+}
+
+define i32 @test_less-precise-fpmad2(i32 %i) "less-precise-fpmad"="true" {
+  %1 = call i32 @less-precise-fpmad_callee0(i32 %i)
+  ret i32 %1
+; CHECK: @test_less-precise-fpmad2(i32 %i) [[FPMAD_FALSE]] {
+; CHECK-NEXT: ret i32
+}
+
+define i32 @test_less-precise-fpmad3(i32 %i) "less-precise-fpmad"="true" {
+  %1 = call i32 @less-precise-fpmad_callee1(i32 %i)
+  ret i32 %1
+; CHECK: @test_less-precise-fpmad3(i32 %i) [[FPMAD_TRUE]] {
+; CHECK-NEXT: ret i32
+}
+
+define i32 @no-implicit-float_callee0(i32 %i) {
+  ret i32 %i
+; CHECK: @no-implicit-float_callee0(i32 %i) {
+; CHECK-NEXT: ret i32
+}
+
+define i32 @no-implicit-float_callee1(i32 %i) noimplicitfloat {
+  ret i32 %i
+; CHECK: @no-implicit-float_callee1(i32 %i) [[NOIMPLICITFLOAT:#[0-9]+]] {
+; CHECK-NEXT: ret i32
+}
+
+define i32 @test_no-implicit-float0(i32 %i) {
+  %1 = call i32 @no-implicit-float_callee0(i32 %i)
+  ret i32 %1
+; CHECK: @test_no-implicit-float0(i32 %i) {
+; CHECK-NEXT: ret i32
+}
+
+define i32 @test_no-implicit-float1(i32 %i) {
+  %1 = call i32 @no-implicit-float_callee1(i32 %i)
+  ret i32 %1
+; CHECK: @test_no-implicit-float1(i32 %i) [[NOIMPLICITFLOAT]] {
+; CHECK-NEXT: ret i32
+}
+
+define i32 @test_no-implicit-float2(i32 %i) noimplicitfloat {
+  %1 = call i32 @no-implicit-float_callee0(i32 %i)
+  ret i32 %1
+; CHECK: @test_no-implicit-float2(i32 %i) [[NOIMPLICITFLOAT]] {
+; CHECK-NEXT: ret i32
+}
+
+define i32 @test_no-implicit-float3(i32 %i) noimplicitfloat {
+  %1 = call i32 @no-implicit-float_callee1(i32 %i)
+  ret i32 %1
+; CHECK: @test_no-implicit-float3(i32 %i) [[NOIMPLICITFLOAT]] {
+; CHECK-NEXT: ret i32
+}
+
+; CHECK: attributes [[FPMAD_FALSE]] = { "less-precise-fpmad"="false" }
+; CHECK: attributes [[FPMAD_TRUE]] = { "less-precise-fpmad"="true" }
+; CHECK: attributes [[NOIMPLICITFLOAT]] = { noimplicitfloat }