[NFC] Include short string attributes in the "Function Attrs" comment

[jdoerfert]

Summary

So far, the comment above a function in LLVM-IR did not include any
string function attributes. While for many of them it is preferable not
to have them there, sometimes it makes sense. This patch introduces a
heuristic based on the size of the string attribute name and value.
If they together do not contain more than 15 characters we include them
in the comment, otherwise we do not.

[rnk]

I'd be in favor of this.

[jdoerfert]

@rnk Should I take that as a LGTM or do you have another person in mind that should take a look?

[pcc]

This change seems kind of arbitrary to me. Not all attributes that we might want to test for are shorter than 15 characters. Some are already longer and they can't be changed because of bitcode compatibility.

If the goal is to make things involving attributes more easily testable, why not work towards that goal directly? For example, you could add a flag that means "print all attribute names/values in the comment" and then use that in your tests.

[jdoerfert]

In D62784#1528498, @pcc wrote:

This change seems kind of arbitrary to me. Not all attributes that we might want to test for are shorter than 15 characters. Some are already longer and they can't be changed because of bitcode compatibility.

While I guess we could introduce short names for existing long attributes, I agree with the general premise, this is "kind of arbitrary". Though, we talk about printing comments and what we do now is arguably "kind of arbitrary" as well. If you feel that it is not a good idea to print more arguments in the comment, I will not do it.

If the goal is to make things involving attributes more easily testable, why not work towards that goal directly? For example, you could add a flag that means "print all attribute names/values in the comment" and then use that in your tests.

I did want to avoid that because we easily have 15+ attributes and new ones are added fairly often. That means testing for all of them is not a good option. Now testing for a subset is hard, especially if you want to ensure some attributes to be there and others to be absent. The way I would write it, potentially not the best way though, would involve multiple interleaved check and check-not lines.

[pcc]

In D62784#1528578, @jdoerfert wrote:

In D62784#1528498, @pcc wrote:

This change seems kind of arbitrary to me. Not all attributes that we might want to test for are shorter than 15 characters. Some are already longer and they can't be changed because of bitcode compatibility.

While I guess we could introduce short names for existing long attributes, I agree with the general premise, this is "kind of arbitrary". Though, we talk about printing comments and what we do now is arguably "kind of arbitrary" as well. If you feel that it is not a good idea to print more arguments in the comment, I will not do it.

If the goal is to make things involving attributes more easily testable, why not work towards that goal directly? For example, you could add a flag that means "print all attribute names/values in the comment" and then use that in your tests.

I did want to avoid that because we easily have 15+ attributes and new ones are added fairly often. That means testing for all of them is not a good option. Now testing for a subset is hard, especially if you want to ensure some attributes to be there and others to be absent. The way I would write it, potentially not the best way though, would involve multiple interleaved check and check-not lines.

Maybe you could sort the attributes by name before printing? Then e.g. if you want to check for presence of "a" and "c" and the absence of "b" you could do

CHECK: "a"
CHECK-NOT: "b"
CHECK: "c"

And it wouldn't matter which other attributes are printed.

[jdoerfert]

In D62784#1531329, @pcc wrote:

In D62784#1528578, @jdoerfert wrote:

In D62784#1528498, @pcc wrote:

This change seems kind of arbitrary to me. Not all attributes that we might want to test for are shorter than 15 characters. Some are already longer and they can't be changed because of bitcode compatibility.

While I guess we could introduce short names for existing long attributes, I agree with the general premise, this is "kind of arbitrary". Though, we talk about printing comments and what we do now is arguably "kind of arbitrary" as well. If you feel that it is not a good idea to print more arguments in the comment, I will not do it.

If the goal is to make things involving attributes more easily testable, why not work towards that goal directly? For example, you could add a flag that means "print all attribute names/values in the comment" and then use that in your tests.

I did want to avoid that because we easily have 15+ attributes and new ones are added fairly often. That means testing for all of them is not a good option. Now testing for a subset is hard, especially if you want to ensure some attributes to be there and others to be absent. The way I would write it, potentially not the best way though, would involve multiple interleaved check and check-not lines.

Maybe you could sort the attributes by name before printing? Then e.g. if you want to check for presence of "a" and "c" and the absence of "b" you could do

CHECK: "a"
CHECK-NOT: "b"
CHECK: "c"

And it wouldn't matter which other attributes are printed.

I agree this would be possible but I'm not sure if it'll be better than what we have now and worth the effort. My solution did break 4 existing tests that add arguments that should not exist in the first place (not included in the patch because I saw them only after a rebase yesterday). I expect the change you propose to cause a lot of breakages. I'll think about it and either update the patch or abandon it.

[jdoerfert]

@pcc So I thought about this. I don't think printing all attributes always helps. Testing for a single "Function Attrs" line will be very brittle, the use of multiple check lines will make tests harder to read/write/maintain and the IR output is cluttered. What if we have an output like this one under a flag? It would have all the benefits this patch has and, as you seem to dislike it, it would be on an opt-in basis. Though, I'm still unsure under which circumstance this patch would be worse than the status quo.

[jdoerfert]

I give up on this for now. If there is interest in this we can revive it later.

llvm/lib/IR/AsmWriter.cpp

if (F->isMaterializable())
     Out << "; Materializable\n";
 
   const AttributeList &Attrs = F->getAttributes();
   if (Attrs.hasAttributes(AttributeList::FunctionIndex)) {
     AttributeSet AS = Attrs.getFnAttributes();
     std::string AttrStr;
 
     for (const Attribute &Attr : AS) {
-      if (!Attr.isStringAttribute()) {
-        if (!AttrStr.empty()) AttrStr += ' ';
-        AttrStr += Attr.getAsString();
-      }
+      bool IsStringAttr = Attr.isStringAttribute();
+      auto AttrKeyStr = Attr.getAsString();
+      // Filter string attributes based on the size of key + value. This is a heuristic.
+      if (IsStringAttr && (AttrKeyStr.size() > 15 + /* quotes */ 4 + /* equal sign */ 1))
+        continue;
+      if (!AttrStr.empty())
+        AttrStr += ' ';
+      AttrStr += AttrKeyStr;
     }
 
     if (!AttrStr.empty())
       Out << "; Function Attrs: " << AttrStr << '\n';
   }
 
   Machine.incorporateFunction(F);
 

llvm/test/Bitcode/attributes.ll

 }
 
 declare void @nobuiltin()
 
 define void @f34()
 ; CHECK: define void @f34()
 {
         call void @nobuiltin() nobuiltin
-; CHECK: call void @nobuiltin() #36
+; CHECK: call void @nobuiltin() #37
         ret void;
 }
 
 define void @f35() optnone noinline
 ; CHECK: define void @f35() #23
 {
         ret void;
 }

 }
 
 ; CHECK: define void @f59() #35
 define void @f59() shadowcallstack
 {
   ret void
 }
 
+; CHECK:      Function Attrs: "short"="option"
+; CHECK-NOT:  long
+; CHECK-NEXT: define void @f60() #36
+define void @f60() #0
+{
+  ret void
+}
+
 ; CHECK: attributes #0 = { noreturn }
 ; CHECK: attributes #1 = { nounwind }
 ; CHECK: attributes #2 = { readnone }
 ; CHECK: attributes #3 = { readonly }
 ; CHECK: attributes #4 = { noinline }
 ; CHECK: attributes #5 = { alwaysinline }
 ; CHECK: attributes #6 = { optsize }
 ; CHECK: attributes #7 = { ssp }

 ; CHECK: attributes #28 = { inaccessiblememonly }
 ; CHECK: attributes #29 = { inaccessiblemem_or_argmemonly }
 ; CHECK: attributes #30 = { allocsize(0) }
 ; CHECK: attributes #31 = { allocsize(0,1) }
 ; CHECK: attributes #32 = { writeonly }
 ; CHECK: attributes #33 = { speculatable }
 ; CHECK: attributes #34 = { sanitize_hwaddress }
 ; CHECK: attributes #35 = { shadowcallstack }
-; CHECK: attributes #36 = { nobuiltin }
+; CHECK: attributes #36 = { "long"="options_are_excluded" "short"="option" }
+; CHECK: attributes #37 = { nobuiltin }
+
+attributes #0 = { "long"="options_are_excluded" "short"="option" }