[IR][AutoUpgrade] Drop alignment from non-pointer parameters and returns

[steven_wu]

Summary

This is a follow-up of D102201. After some discussion, it is a better idea
to upgrade all invalid uses of alignment attributes on function return
values and parameters, not just limited to void function return types.

[jdoerfert]

First, thanks a lot for looking into this again. I appreciate it a lot.

I left a bunch of minor comments and suggestions. Now that I thought about it for a second, I feel we should not look for alignment at all but instead call typeIncompatible here. For one, that will make future updates obsolete. It also will directly handle other attributes we might have missed here. Finally, it will handle pointer vectors and such properly.

WDYT?

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

5641

I didn't see this before but CallInst is not enough, you want CallBase.
I'm not sure why you go through the functiontype but it seems odd.
If the call base type is not a pointer, just remove alignment attribute (no need to check I think).
You want the call site arguments not the function type parameters, the former are potentially more for varargs functions and then we also don't want to have aligned non-pointer arguments.
And again, no need to check if the argument is present, removing it eagerly should be as expensive.

llvm/lib/IR/AutoUpgrade.cpp

4383–4388

I haven't run this but should be easier to read.

[steven_wu]

Address review feedback. Now remove all incompatible attributes from non pointer type.

Maybe we can expand more to try to upgrade everything including pointer type as well.

[steven_wu]

Remove all incompatible attributes.

[jdoerfert]

LGTM, thanks for fixing this now and for the future!

[dexonsmith]

Can you include the revert to attributes-3.3.ll.bc as part of this?

[steven_wu]

In D102726#2769821, @dexonsmith wrote:

Can you include the revert to attributes-3.3.ll.bc as part of this?

I don't think that matters too much and I am not sure if I revert a binary file will add a duplication to git history. The case in 3.3 version is kind of covered by newly added test case except created by different llvm version.

[jdoerfert]

Nit: Commit subject (not message) is old, maybe change prior to push locally

[steven_wu]

In D102726#2769935, @jdoerfert wrote:

Nit: Commit subject (not message) is old, maybe change prior to push locally

I didn't update the PR description but you can see my actual commit message by go to Revision Contents -> Commits -> Summary (show more..)

[dexonsmith]

In D102726#2769856, @steven_wu wrote:

In D102726#2769821, @dexonsmith wrote:

Can you include the revert to attributes-3.3.ll.bc as part of this?

I don't think that matters too much

I think it matters. With the original file in place, it's clear that we actually upgrade correctly all the attributes that were in the 3.3 test. Without it, it's unclear.

I am not sure if I revert a binary file will add a duplication to git history.

Every object in Git is referenced by the hash of its content. Older commits (before @jdoerfert's patch) will point at the same hash for attributes-3.3.ll.bc as the new commits after it gets reverted back. There's no duplication here.

The case in 3.3 version is kind of covered by newly added test case except created by different llvm version.

Yeah, kind of. But not entirely.

[steven_wu]

In D102726#2771933, @dexonsmith wrote:

In D102726#2769856, @steven_wu wrote:

In D102726#2769821, @dexonsmith wrote:

Can you include the revert to attributes-3.3.ll.bc as part of this?

I don't think that matters too much

I think it matters. With the original file in place, it's clear that we actually upgrade correctly all the attributes that were in the 3.3 test. Without it, it's unclear.

I am not sure if I revert a binary file will add a duplication to git history.

Every object in Git is referenced by the hash of its content. Older commits (before @jdoerfert's patch) will point at the same hash for attributes-3.3.ll.bc as the new commits after it gets reverted back. There's no duplication here.

The case in 3.3 version is kind of covered by newly added test case except created by different llvm version.

Yeah, kind of. But not entirely.

I will do that as a followup.

[dexonsmith]

In D102726#2771958, @steven_wu wrote:

I will do that as a followup.

Thanks!

[nikic]

FYI, this change causes a non-trivial compile-time regression in LTO builds: https://llvm-compile-time-tracker.com/compare.php?from=136ced498ba84f6b6126051626e319f18ba740f5&to=5b6cae5524905bc43cfc21a515f828528d1f2e68&stat=instructions (Worst regression seems to be the ThinLTO kc.link step, which is up 4%.)

LLVM attributes are optimized for immutable usage, and it's hard to overstate just how slow the mutable APIs on them are...

[jdoerfert]

In D102726#2774657, @nikic wrote:

FYI, this change causes a non-trivial compile-time regression in LTO builds: https://llvm-compile-time-tracker.com/compare.php?from=136ced498ba84f6b6126051626e319f18ba740f5&to=5b6cae5524905bc43cfc21a515f828528d1f2e68&stat=instructions (Worst regression seems to be the ThinLTO kc.link step, which is up 4%.)

LLVM attributes are optimized for immutable usage, and it's hard to overstate just how slow the mutable APIs on them are...

I suggested not to check for attributes just to remove them. I still think that's the right thing *but* we can check for them in the remove functions.
@nikic you think that will resolve the compile time issue?

[dexonsmith]

In D102726#2774689, @jdoerfert wrote:

In D102726#2774657, @nikic wrote:

FYI, this change causes a non-trivial compile-time regression in LTO builds: https://llvm-compile-time-tracker.com/compare.php?from=136ced498ba84f6b6126051626e319f18ba740f5&to=5b6cae5524905bc43cfc21a515f828528d1f2e68&stat=instructions (Worst regression seems to be the ThinLTO kc.link step, which is up 4%.)

LLVM attributes are optimized for immutable usage, and it's hard to overstate just how slow the mutable APIs on them are...

I suggested not to check for attributes just to remove them. I still think that's the right thing *but* we can check for them in the remove functions.
@nikic you think that will resolve the compile time issue?

Three other ideas, which can all be applied, but the compile time regression might be fixed without doing them all:

1. Serialize somewhere an "attribute list version", which can be used to skip attribute-upgrading entirely if the current and serialized versions match. This avoids any slowdown in the typical LTO case (old archived object files notwithstanding).

2. Keep a map when upgrading from (AttributeList,FunctionType*) to AttributeList to cache the computation.

3. Construct a new attribute list in bulk rather than going through intermediate states and mutating the old one piecemeal.

[nikic]

In D102726#2774689, @jdoerfert wrote:

In D102726#2774657, @nikic wrote:

FYI, this change causes a non-trivial compile-time regression in LTO builds: https://llvm-compile-time-tracker.com/compare.php?from=136ced498ba84f6b6126051626e319f18ba740f5&to=5b6cae5524905bc43cfc21a515f828528d1f2e68&stat=instructions (Worst regression seems to be the ThinLTO kc.link step, which is up 4%.)

LLVM attributes are optimized for immutable usage, and it's hard to overstate just how slow the mutable APIs on them are...

I suggested not to check for attributes just to remove them. I still think that's the right thing *but* we can check for them in the remove functions.
@nikic you think that will resolve the compile time issue?

I've added two optimizations along those lines in https://github.com/llvm/llvm-project/commit/fd46ed3f397d6cf41bc6c5a04ab2089f585afe44 and https://github.com/llvm/llvm-project/commit/05738ffcb87b76c6f166f965ba9b2db3257a4338.

The remaining impact from this change is now https://llvm-compile-time-tracker.com/compare.php?stat=instructions&from=9530d41b3106a8bdc619f7bf4dd6e2e645bb2b07&to=05738ffcb87b76c6f166f965ba9b2db3257a4338, which seems acceptable.

[steven_wu]

Just saw this. Thanks for taking care of the fallout.

[jdoerfert]

Yep, thanks to both of you to solving the problem with general solutions, will help in the long run for sure :)

llvm/include/llvm/IR/Argument.h

public:
 
   void addAttr(Attribute::AttrKind Kind);
 
   void addAttr(Attribute Attr);
 
   /// Remove attributes from an argument.
   void removeAttr(Attribute::AttrKind Kind);
 
+  void removeAttrs(const AttrBuilder &B);
+
   /// Check if an argument has a given attribute.
   bool hasAttribute(Attribute::AttrKind Kind) const;
 
   Attribute getAttribute(Attribute::AttrKind Kind) const;
 
   /// Method for support type inquiry through isa, cast, and dyn_cast.
   static bool classof(const Value *V) {
     return V->getValueID() == ArgumentVal;
   }
 };
 
 } // End llvm namespace
 
 #endif

llvm/include/llvm/IR/InstrTypes.h

public:
   /// Removes the attribute from the given argument
   void removeParamAttr(unsigned ArgNo, StringRef Kind) {
     assert(ArgNo < getNumArgOperands() && "Out of bounds");
     AttributeList PAL = getAttributes();
     PAL = PAL.removeParamAttribute(getContext(), ArgNo, Kind);
     setAttributes(PAL);
   }
 
+  /// Removes the attributes from the given argument
+  void removeParamAttrs(unsigned ArgNo, const AttrBuilder &Attrs) {
+    AttributeList PAL = getAttributes();
+    PAL = PAL.removeParamAttributes(getContext(), ArgNo, Attrs);
+    setAttributes(PAL);
+  }
+
   /// Removes noundef and other attributes that imply undefined behavior if a
   /// `undef` or `poison` value is passed from the given argument.
   void removeParamUndefImplyingAttrs(unsigned ArgNo) {
     assert(ArgNo < getNumArgOperands() && "Out of bounds");
     AttributeList PAL = getAttributes();
     PAL = PAL.removeParamUndefImplyingAttributes(getContext(), ArgNo);
     setAttributes(PAL);
   }

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

if (auto *MD = I.getMetadata(LLVMContext::MD_prof)) {
           continue; // ignore and continue.
 
         // If branch weight doesn't match, just strip branch weight.
         if (MD->getNumOperands() != 1 + ExpectedNumOperands)
           I.setMetadata(LLVMContext::MD_prof, nullptr);
       }
     }
 
-    // Remove align from return attribute on CallInst.
-    if (auto *CI = dyn_cast<CallInst>(&I)) {
-      if (CI->getFunctionType()->getReturnType()->isVoidTy())
-        CI->removeAttribute(0, Attribute::Alignment);
+    // Remove incompatible attributes on function calls.
+    if (auto *CI = dyn_cast<CallBase>(&I)) {
+      CI->removeAttributes(AttributeList::ReturnIndex,
+                           AttributeFuncs::typeIncompatible(
+                               CI->getFunctionType()->getReturnType()));
+
+      for (unsigned ArgNo = 0; ArgNo < CI->arg_size(); ++ArgNo)
+        CI->removeParamAttrs(ArgNo, AttributeFuncs::typeIncompatible(
+                                        CI->getArgOperand(ArgNo)->getType()));
     }
   }
 
   // Look for functions that rely on old function attribute behavior.
   UpgradeFunctionAttributes(*F);
 
   // Bring in any functions that this function forward-referenced via
   // blockaddresses.
   return materializeForwardReferencedFunctions();

llvm/lib/IR/AutoUpgrade.cpp

void llvm::UpgradeFunctionAttributes(Function &F) {
 
   if (F.getCallingConv() == CallingConv::X86_INTR &&
       !F.arg_empty() && !F.hasParamAttribute(0, Attribute::ByVal)) {
     Type *ByValTy = cast<PointerType>(F.getArg(0)->getType())->getElementType();
     Attribute NewAttr = Attribute::getWithByValType(F.getContext(), ByValTy);
     F.addParamAttr(0, NewAttr);
   }
 
-  // If function has void return type, check it has align attribute. It has no
-  // affect on the return type and no longer passes the verifier.
-  if (F.getReturnType()->isVoidTy() &&
-      F.hasAttribute(AttributeList::ReturnIndex, Attribute::Alignment))
-    F.removeAttribute(AttributeList::ReturnIndex, Attribute::Alignment);
+  // Remove all incompatibile attributes from function.
+  F.removeAttributes(AttributeList::ReturnIndex,
+                     AttributeFuncs::typeIncompatible(F.getReturnType()));
+  for (auto &Arg : F.args())
+    Arg.removeAttrs(AttributeFuncs::typeIncompatible(Arg.getType()));
 }
 
 static bool isOldLoopArgument(Metadata *MD) {
   auto *T = dyn_cast_or_null<MDTuple>(MD);
   if (!T)
     return false;
   if (T->getNumOperands() < 1)
     return false;
   auto *S = dyn_cast_or_null<MDString>(T->getOperand(0));
   if (!S)
     return false;
   return S->getString().startswith("llvm.vectorizer.");

llvm/lib/IR/Function.cpp

 void Argument::addAttr(Attribute Attr) {
   getParent()->addParamAttr(getArgNo(), Attr);
 }
 
 void Argument::removeAttr(Attribute::AttrKind Kind) {
   getParent()->removeParamAttr(getArgNo(), Kind);
 }
 
+void Argument::removeAttrs(const AttrBuilder &B) {
+  AttributeList AL = getParent()->getAttributes();
+  AL = AL.removeParamAttributes(Parent->getContext(), getArgNo(), B);
+  getParent()->setAttributes(AL);
+}
+
 bool Argument::hasAttribute(Attribute::AttrKind Kind) const {
   return getParent()->hasParamAttribute(getArgNo(), Kind);
 }
 
 Attribute Argument::getAttribute(Attribute::AttrKind Kind) const {
   return getParent()->getParamAttribute(getArgNo(), Kind);
 }
 

llvm/test/Bitcode/upgrade-incompatible-func-attr-11.0.ll

+; Check upgrade is removing the incompatible attributes on function types.
+
+; RUN: llvm-dis < %s.bc | FileCheck %s
+
+; CHECK: define i8 @f(i8 %0, i8 %1)
+define align 8 i8 @f(i8 align 8 %0, i8 align 8 %1) {
+  ret i8 0
+}
+
+; CHECK: declare i8 @f2(i8, i8, ...)
+declare align 8 i8 @f2(i8 align 8, i8 align 8, ...)
+
+declare i32* @"personality_function"()
+
+define void @g() personality i32* ()* @"personality_function" {
+; CHECK: call i8 @f(i8 0, i8 1)
+  %1 = call align 8 i8 @f(i8 align 8 0, i8 align 8 1);
+; CHECK: call i8 (i8, i8, ...) @f2(i8 0, i8 1, i8 2)
+  %2 = call align 8 i8(i8, i8, ...) @f2(i8 align 8 0, i8 align 8 1, i8 align 8 2);
+; CHECK: invoke i8 @f(i8 0, i8 1)
+  %3 = invoke align 8 i8 @f(i8 align 8 0, i8 align 8 1) to label %cont unwind label %cleanup
+
+cont:
+  ret void
+
+cleanup:
+  %4 = landingpad i8 cleanup
+  ret void
+}