[IR][AutoUpgrade] Drop align attribute from void return types

[steven_wu]

Summary

Since D87304, align become an invalid attribute on none pointer types and
verifier will reject bitcode that has invalid align attribute.

The problem is before the change, DeadArgumentElimination can easily
turn a pointer return type into a void return type without removing
align attribute. Teach Autograde to remove invalid align attribute
from return types to maintain bitcode compatibility.

rdar://77022993

[dexonsmith]

LGTM with a couple of nits about the test.

llvm/test/Bitcode/upgrade-void-ret-attr.ll

1 ↗

(On Diff #344219)

Please add a period at the end of the sentence. Also, I'm not sure we need the ;;, one is probably fine.

3 ↗

(On Diff #344219)

For the bitcode file, I suggest using %s.bc (meaning the extension is .ll.bc), to simplify this.

Also, please rename the .ll file to end with the released version of LLVM used to generate the bitcode (e.g., -10.0.ll).

[steven_wu]

Address review feedback.

[thakis]

We bisected a +153kB binary size increase for chrome/android to this (https://bugs.chromium.org/p/chromium/issues/detail?id=1208513#c4). Is that expected? (That build config uses -Oz for many TUs.)

[dexonsmith]

In D102201#2761154, @thakis wrote:

We bisected a +153kB binary size increase for chrome/android to this (https://bugs.chromium.org/p/chromium/issues/detail?id=1208513#c4). Is that expected? (That build config uses -Oz for many TUs.)

Hmm, that seems pretty weird — this commit fixes a compiler crash introduced by a missing bitcode upgrade from https://reviews.llvm.org/D87304, they should be reverted together if we need to revert something.

@jdoerfert, any thoughts on why removing the align attribute causes a regression here?

@thakis, is it possible the Chromium build is skipping the bitcode verifier when reading bitcode?

[thakis]

In D102201#2761403, @dexonsmith wrote:

In D102201#2761154, @thakis wrote:

We bisected a +153kB binary size increase for chrome/android to this (https://bugs.chromium.org/p/chromium/issues/detail?id=1208513#c4). Is that expected? (That build config uses -Oz for many TUs.)

Hmm, that seems pretty weird — this commit fixes a compiler crash introduced by a missing bitcode upgrade from https://reviews.llvm.org/D87304, they should be reverted together if we need to revert something.

@jdoerfert, any thoughts on why removing the align attribute causes a regression here?

@thakis, is it possible the Chromium build is skipping the bitcode verifier when reading bitcode?

As far as I can tell we don't do anything clever in this area. We build clang without assertions and then use -flto=thin. What would cause clang to skip the bitcode verifier?

[jdoerfert]

I can see how this increases IR size but I'm not sure about binary size. If you have your IR in the binary, then that is probably it. If not, I would assume it's some secondary effect that should not exist.

FWIW, IR size increase:

declare align 8 void @f() willreturn;
declare align 8 int *@g() willreturn;

share the same attribute set but if you remove align 8 from @f we have two distinct attribute sets in the IR.

---

I would not revert D87304 and this one to undo the binary size increase. Both patches fix assertions that can hit in practice.
We need to understand what caused the increase and target that part. Allowing align X void on returns is not a fix.

[steven_wu]

I don’t understand code size increase as well and have no guess about what might happen. This should be noop on bitcode that can pass the verifier.

Are you able to reduce it further to a TU and have a reproducer?

[aeubanks]

The size regression disappeared in our latest Clang roll
Reverting this patch at Clang head doesn't seem to cause any size changes, and yeah this doesn't seem like it should cause a difference.
So I'm not sure why the bisection ended up here (each step takes ~1hr so it's painful to reproduce). Sorry for the noise.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

for (auto &I : instructions(F)) {
       MDNode *TBAA = I.getMetadata(LLVMContext::MD_tbaa);
       if (!TBAA || TBAAVerifyHelper.visitTBAAMetadata(I, TBAA))
         continue;
       MDLoader->setStripTBAA(true);
       stripTBAA(F->getParent());
     }
   }
 
-  // "Upgrade" older incorrect branch weights by dropping them.
   for (auto &I : instructions(F)) {
+    // "Upgrade" older incorrect branch weights by dropping them.
     if (auto *MD = I.getMetadata(LLVMContext::MD_prof)) {
       if (MD->getOperand(0) != nullptr && isa<MDString>(MD->getOperand(0))) {
         MDString *MDS = cast<MDString>(MD->getOperand(0));
         StringRef ProfName = MDS->getString();
         // Check consistency of !prof branch_weights metadata.
         if (!ProfName.equals("branch_weights"))
           continue;
         unsigned ExpectedNumOperands = 0;
if (auto *MD = I.getMetadata(LLVMContext::MD_prof)) {
         else
           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);
+    }
   }
 
   // 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);
 }
 
 static bool isOldLoopArgument(Metadata *MD) {
   auto *T = dyn_cast_or_null<MDTuple>(MD);
   if (!T)
     return false;
   if (T->getNumOperands() < 1)
     return false;

llvm/test/Bitcode/upgrade-void-ret-attr-11.0.ll

+; Check upgrade is removing the incompatible attributes on void return type.
+
+; RUN: llvm-dis < %s.bc | FileCheck %s
+
+; CHECK: define void @f()
+define align 8 void @f() {
+  ret void
+}
+
+define void @g() {
+; CHECK: call void @f()
+  call align 8 void @f();
+  ret void
+}