[InstCombineCalls] Optimize call of bitcast even w/ parameter attributes

[jdoerfert]

Summary

Before we gave up if a call through bitcast had parameter attributes.
Interestingly, we allowed attributes for the return value already. We
now handle both the same way, namely, we drop the ones that are
incompatible with the new type and keep the rest. This cannot cause
"more UB" than initially present.

[nikic]

I'm not convinced this is right when there are ABI affecting attributes involved. Consider the first example, where zeroext is dropped. Let's hypothetically assume that this indicates a zeroext up to 64-bit, but we have 32-bit pointers. Doing the transform and dropping the zeroext means that the top bits will be left uninitialized now.

For non-ABI attributes, I agree that just dropping them is fine.

[jdoerfert]

In D119967#3327278, @nikic wrote:

I'm not convinced this is right when there are ABI affecting attributes involved. Consider the first example, where zeroext is dropped. Let's hypothetically assume that this indicates a zeroext up to 64-bit, but we have 32-bit pointers. Doing the transform and dropping the zeroext means that the top bits will be left uninitialized now.

I don't think zeroext is a problem. The 64 bit case shows why, isBitOrNoopPointerCastable is false and we won't transform it.

For non-ABI attributes, I agree that just dropping them is fine.

[aeubanks]

llvm/test/Transforms/InstCombine/call-cast-attrs.ll

36

is losing the nocapture worth changing this to a direct call?

[jdoerfert]

llvm/test/Transforms/InstCombine/call-cast-attrs.ll

36

Yes, because we cannot analyze or inline indirect calls. The nocapture here would not be derived by any pass.

[nikic]

In D119967#3327289, @jdoerfert wrote:

In D119967#3327278, @nikic wrote:

I'm not convinced this is right when there are ABI affecting attributes involved. Consider the first example, where zeroext is dropped. Let's hypothetically assume that this indicates a zeroext up to 64-bit, but we have 32-bit pointers. Doing the transform and dropping the zeroext means that the top bits will be left uninitialized now.

I don't think zeroext is a problem. The 64 bit case shows why, isBitOrNoopPointerCastable is false and we won't transform it.

I'm referring to the case where the argument is pointer-sized (so it is castable), but the (target-specific) semantics of zeroext require an extension beyond pointer size.

[jdoerfert]

In D119967#3336985, @nikic wrote:

In D119967#3327289, @jdoerfert wrote:

In D119967#3327278, @nikic wrote:

I'm not convinced this is right when there are ABI affecting attributes involved. Consider the first example, where zeroext is dropped. Let's hypothetically assume that this indicates a zeroext up to 64-bit, but we have 32-bit pointers. Doing the transform and dropping the zeroext means that the top bits will be left uninitialized now.

I don't think zeroext is a problem. The 64 bit case shows why, isBitOrNoopPointerCastable is false and we won't transform it.

I'm referring to the case where the argument is pointer-sized (so it is castable), but the (target-specific) semantics of zeroext require an extension beyond pointer size.

So you are saying it is legal and expected that we extend the argument even though the callee expects a pointer (of smaller size)? (How is that not UB?)

[jdoerfert]

friendly ping :)

[efriedma]

In general, the reason we don't just treat calling a bitcast as instant UB is that we want to avoid breaking cases where different signatures represent equivalent calls, but the IR types aren't compatible. We don't have any standard rule for lowering C calls to IR, so different language frontends might do slightly different things.

Along those lines, some attributes hide significant differences in how the call is actually lowered; for example, byval means that a "pointer" isn't really a pointer at all. Some attributes are obviously irrelevant, like noundef.

Going through the LangRef list, zeroext/signext/inreg/sret/nest/swiftself/swiftasync/swifterror are the parameter attributes which this code currently doesn't explicitly check for, but are ABI significant. But... given the motivation, they're unlikely to be relevant, I guess? If there's a mismatch, and the call otherwise passes the checks, the untransformed call is probably UB anyway.

That said, I don't really want to try to write up a coherent justification for each of those, or worry about attributes that will be implemented in the future. An explicit list of attributes which are obviously safe to drop would be much simpler to reason about.

[jdoerfert]

Introduce the concept of safe and unsafe attributes wrt. dropping them

[lebedev.ri]

In D119967#3410527, @jdoerfert wrote:

Introduce the concept of safe and unsafe attributes wrt. dropping them

Nice!

[efriedma]

LGTM

The classification into safe/unsafe makes sense to me. There's probably some further cleanup we could do here, but this seems like a reasonable intermediate step.

llvm/include/llvm/IR/Attributes.h

public:
   ArrayRef<Attribute> attrs() const { return Attrs; }
 
   bool operator==(const AttrBuilder &B) const;
   bool operator!=(const AttrBuilder &B) const { return !(*this == B); }
 };
 
 namespace AttributeFuncs {
 
-/// Which attributes cannot be applied to a type.
-AttributeMask typeIncompatible(Type *Ty);
+enum AttributeSafetyKind : uint8_t {
+  ASK_SAFE_TO_DROP = 1,
+  ASK_UNSAFE_TO_DROP = 2,
+  ASK_ALL = ASK_SAFE_TO_DROP | ASK_UNSAFE_TO_DROP,
+};
+
+/// Which attributes cannot be applied to a type. The argument \p ASK indicates,
+/// if only attributes that are known to be safely droppable are contained in
+/// the mask; only attributes that might be unsafe to drop (e.g., ABI-related
+/// attributes) are in the mask; or both.
+AttributeMask typeIncompatible(Type *Ty, AttributeSafetyKind ASK = ASK_ALL);
 
 /// Get param/return attributes which imply immediate undefined behavior if an
 /// invalid value is passed. For example, this includes noundef (where undef
 /// implies UB), but not nonnull (where null implies poison). It also does not
 /// include attributes like nocapture, which constrain the function
 /// implementation rather than the passed value.
 AttributeMask getUBImplyingAttributes();
 

llvm/lib/IR/Attributes.cpp

bool AttrBuilder::operator==(const AttrBuilder &B) const {
   return Attrs == B.Attrs;
 }
 
 //===----------------------------------------------------------------------===//
 // AttributeFuncs Function Defintions
 //===----------------------------------------------------------------------===//
 
 /// Which attributes cannot be applied to a type.
-AttributeMask AttributeFuncs::typeIncompatible(Type *Ty) {
+AttributeMask AttributeFuncs::typeIncompatible(Type *Ty,
+                                               AttributeSafetyKind ASK) {
   AttributeMask Incompatible;
 
-  if (!Ty->isIntegerTy())
+  if (!Ty->isIntegerTy()) {
     // Attributes that only apply to integers.
-    Incompatible.addAttribute(Attribute::SExt)
-        .addAttribute(Attribute::ZExt)
-        .addAttribute(Attribute::AllocAlign);
+    if (ASK & ASK_SAFE_TO_DROP)
+      Incompatible.addAttribute(Attribute::AllocAlign);
+    if (ASK & ASK_UNSAFE_TO_DROP)
+      Incompatible.addAttribute(Attribute::SExt).addAttribute(Attribute::ZExt);
+  }
 
-  if (!Ty->isPointerTy())
+  if (!Ty->isPointerTy()) {
     // Attributes that only apply to pointers.
-    Incompatible.addAttribute(Attribute::Nest)
-        .addAttribute(Attribute::NoAlias)
+    if (ASK & ASK_SAFE_TO_DROP)
+      Incompatible.addAttribute(Attribute::NoAlias)
           .addAttribute(Attribute::NoCapture)
           .addAttribute(Attribute::NonNull)
           .addAttribute(Attribute::ReadNone)
           .addAttribute(Attribute::ReadOnly)
-        .addAttribute(Attribute::SwiftError)
           .addAttribute(Attribute::Dereferenceable)
-        .addAttribute(Attribute::DereferenceableOrNull)
+          .addAttribute(Attribute::DereferenceableOrNull);
+    if (ASK & ASK_UNSAFE_TO_DROP)
+      Incompatible.addAttribute(Attribute::Nest)
+          .addAttribute(Attribute::SwiftError)
           .addAttribute(Attribute::Preallocated)
           .addAttribute(Attribute::InAlloca)
           .addAttribute(Attribute::ByVal)
           .addAttribute(Attribute::StructRet)
           .addAttribute(Attribute::ByRef)
           .addAttribute(Attribute::ElementType);
+  }
 
-  if (!Ty->isPtrOrPtrVectorTy())
     // Attributes that only apply to pointers or vectors of pointers.
+  if (!Ty->isPtrOrPtrVectorTy()) {
+    if (ASK & ASK_SAFE_TO_DROP)
       Incompatible.addAttribute(Attribute::Alignment);
+  }
 
   // Some attributes can apply to all "values" but there are no `void` values.
-  if (Ty->isVoidTy())
+  if (Ty->isVoidTy()) {
+    if (ASK & ASK_SAFE_TO_DROP)
       Incompatible.addAttribute(Attribute::NoUndef);
+  }
 
   return Incompatible;
 }
 
 AttributeMask AttributeFuncs::getUBImplyingAttributes() {
   AttributeMask AM;
   AM.addAttribute(Attribute::NoUndef);
   AM.addAttribute(Attribute::Dereferenceable);

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) {
   auto AI = Call.arg_begin();
   for (unsigned i = 0, e = NumCommonArgs; i != e; ++i, ++AI) {
     Type *ParamTy = FT->getParamType(i);
     Type *ActTy = (*AI)->getType();
 
     if (!CastInst::isBitOrNoopPointerCastable(ActTy, ParamTy, DL))
       return false;   // Cannot transform this parameter value.
 
+    // Check if there are any incompatible attributes we cannot drop safely.
     if (AttrBuilder(FT->getContext(), CallerPAL.getParamAttrs(i))
-            .overlaps(AttributeFuncs::typeIncompatible(ParamTy)))
+            .overlaps(AttributeFuncs::typeIncompatible(
+                ParamTy, AttributeFuncs::ASK_UNSAFE_TO_DROP)))
       return false;   // Attribute not compatible with transformed value.
 
-    if (Call.isInAllocaArgument(i))
-      return false;   // Cannot transform to and from inalloca.
+    if (Call.isInAllocaArgument(i) ||
+        CallerPAL.hasParamAttr(i, Attribute::Preallocated))
+      return false; // Cannot transform to and from inalloca/preallocated.
 
     if (CallerPAL.hasParamAttr(i, Attribute::SwiftError))
       return false;
 
     // If the parameter is passed as a byval argument, then we have to have a
     // sized type and the sized type has to have the same size as the old type.
     if (ParamTy != ActTy && CallerPAL.hasParamAttr(i, Attribute::ByVal)) {
       PointerType *ParamPTy = dyn_cast<PointerType>(ParamTy);
bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) {
   for (unsigned i = 0; i != NumCommonArgs; ++i, ++AI) {
     Type *ParamTy = FT->getParamType(i);
 
     Value *NewArg = *AI;
     if ((*AI)->getType() != ParamTy)
       NewArg = Builder.CreateBitOrPointerCast(*AI, ParamTy);
     Args.push_back(NewArg);
 
-    // Add any parameter attributes.
+    // Add any parameter attributes except the ones incompatible with the new
+    // type. Note that we made sure all incompatible ones are safe to drop.
+    AttributeMask IncompatibleAttrs = AttributeFuncs::typeIncompatible(
+        ParamTy, AttributeFuncs::ASK_SAFE_TO_DROP);
     if (CallerPAL.hasParamAttr(i, Attribute::ByVal) &&
         !ParamTy->isOpaquePointerTy()) {
-      AttrBuilder AB(FT->getContext(), CallerPAL.getParamAttrs(i));
-      AB.addByValAttr(ParamTy->getNonOpaquePointerElementType());
+      AttrBuilder AB(Ctx, CallerPAL.getParamAttrs(i).removeAttributes(
+                              Ctx, IncompatibleAttrs));
+      AB.addByValAttr(NewArg->getType()->getPointerElementType());
       ArgAttrs.push_back(AttributeSet::get(Ctx, AB));
-    } else
-      ArgAttrs.push_back(CallerPAL.getParamAttrs(i));
+    } else {
+      ArgAttrs.push_back(
+          CallerPAL.getParamAttrs(i).removeAttributes(Ctx, IncompatibleAttrs));
+    }
   }
 
   // If the function takes more arguments than the call was taking, add them
   // now.
   for (unsigned i = NumCommonArgs; i != FT->getNumParams(); ++i) {
     Args.push_back(Constant::getNullValue(FT->getParamType(i)));
     ArgAttrs.push_back(AttributeSet());
   }

llvm/test/Transforms/InstCombine/call-cast-attrs.ll

-; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+; RUN: opt < %s -passes=instcombine -data-layout="p:32:32" -S | FileCheck %s --check-prefixes=CHECK,CHECK32
+; RUN: opt < %s -passes=instcombine -data-layout="p:64:64" -S | FileCheck %s --check-prefixes=CHECK,CHECK64
 
 define signext i32 @b(i32* inreg %x)   {
   ret i32 0
 }
 
 define void @c(...) {
   ret void
 }
 
 declare void @useit(i32)
 
 define void @d(i32 %x, ...) {
   call void @useit(i32 %x)
   ret void
 }
 
 define void @g(i32* %y) {
   call i32 bitcast (i32 (i32*)* @b to i32 (i32)*)(i32 zeroext 0)
   call void bitcast (void (...)* @c to void (i32*)*)(i32* %y)
   call void bitcast (void (...)* @c to void (i32*)*)(i32* sret(i32) %y)
   call void bitcast (void (i32, ...)* @d to void (i32, i32*)*)(i32 0, i32* sret(i32) %y)
+  call void bitcast (void (i32, ...)* @d to void (i32, i32*)*)(i32 0, i32* nocapture %y)
+  call void bitcast (void (i32, ...)* @d to void (i32*)*)(i32* nocapture noundef %y)
   ret void
 }
 ; CHECK-LABEL: define void @g(i32* %y)
-; CHECK: call i32 bitcast (i32 (i32*)* @b to i32 (i32)*)(i32 zeroext 0)
-; CHECK: call void (...) @c(i32* %y)
-; CHECK: call void bitcast (void (...)* @c to void (i32*)*)(i32* sret(i32) %y)
-; CHECK: call void bitcast (void (i32, ...)* @d to void (i32, i32*)*)(i32 0, i32* sret(i32) %y)
+; CHECK:    call i32 bitcast (i32 (i32*)* @b to i32 (i32)*)(i32 zeroext 0)
+; CHECK:    call void (...) @c(i32* %y)
+; CHECK:    call void bitcast (void (...)* @c to void (i32*)*)(i32* sret(i32) %y)
+; CHECK:    call void bitcast (void (i32, ...)* @d to void (i32, i32*)*)(i32 0, i32* sret(i32) %y)
+; CHECK:    call void (i32, ...) @d(i32 0, i32* nocapture %y)
+; CHECK32:  %2 = ptrtoint i32* %y to i32
+; CHECK32:  call void (i32, ...) @d(i32 noundef %2)
+; CHECK64:  call void bitcast (void (i32, ...)* @d to void (i32*)*)(i32* nocapture noundef %y)