[AttributeFuncs] Consider `align` in `typeIncompatible`

[jdoerfert]

Summary

Alignment attributes need to be dropped for non-pointer values.
This also introduces a check into the verifier to ensure you don't use
align on anything but a pointer. Test needed to be adjusted
accordingly.

[jdoerfert]

@serge-sans-paille did update the .bc file, many thanks!

[fhahn]

llvm/test/Bitcode/attributes-3.3.ll

105

just to clarify, attributes-3.3.ll.bc needs to be updated, because it contains bitcode that defines a function with align on a non-pointer argument?

[jdoerfert]

llvm/test/Bitcode/attributes-3.3.ll

105

it was updated because of that, yes.

[fhahn]

LGTM, thanks!

[dantrushin]

I wonder if align attribute is legal for vectors of pointers?
This is allowed:

define align 8 i32* @test_scalar(i32* %in) {
  ret i32* %in
}

but this is not:

define align 8 <4 x i32*> @test_vector(<4 x i32*> %in) {
  ret <4 x i32*> %in
}

Is it intentional behavior or just has been overlooked?

[jdoerfert]

In D87304#2318780, @dantrushin wrote:

I wonder if align attribute is legal for vectors of pointers?
This is allowed:

define align 8 i32* @test_scalar(i32* %in) {
  ret i32* %in
}

but this is not:

define align 8 <4 x i32*> @test_vector(<4 x i32*> %in) {
  ret <4 x i32*> %in
}

Is it intentional behavior or just has been overlooked?

The way I read the lang ref this is intentional. Not to say one should not define what it means for vectors of pointers and then allow it. I feel align is not the only one missing for them though.
You want to give it a try?

[dantrushin]

The way I read the lang ref this is intentional. Not to say one should not define what it means for vectors of pointers and then allow it. I feel align is not the only one missing for them though.
You want to give it a try?

Not yet. We have a downstream code which generates this (align on vector of pointers) . I'm trying to figure out where I need to fix it :)
If we really need this, I'll give it a try.

[anna]

In D87304#2319424, @jdoerfert wrote:

In D87304#2318780, @dantrushin wrote:

I wonder if align attribute is legal for vectors of pointers?
This is allowed:

define align 8 i32* @test_scalar(i32* %in) {
  ret i32* %in
}

but this is not:

define align 8 <4 x i32*> @test_vector(<4 x i32*> %in) {
  ret <4 x i32*> %in
}

Is it intentional behavior or just has been overlooked?

The way I read the lang ref this is intentional. Not to say one should not define what it means for vectors of pointers and then allow it. I feel align is not the only one missing for them though.
You want to give it a try?

@jdoerfert I'm wondering why we need to specially treat vector of pointers? We can add a LangRef statement for this, but vector is just another type. The way I read it, whatever is done for the scalar type, we do so for the vector type by considering each element.
So, in this case, the align 8 <4 x i32*> implies that each pointer in the vector is aligned by 8 bytes.

[jdoerfert]

@jdoerfert I'm wondering why we need to specially treat vector of pointers? We can add a LangRef statement for this, but vector is just another type. The way I read it, whatever is done for the scalar type, we do so for the vector type by considering each element. So, in this case, the align 8 <4 x i32*> implies that each pointer in the vector is aligned by 8 bytes.

I don't read it that way because it is not clear this is the case/what we want for all attributes. We cannot silently pick and choose when attributes are applied element wise and when not. Take

`align 8 inalloca noundef <4 x i32*>`

for which I do not know what applies element wise and what applies to the vector value. TBH, maybe we need both ...

A reasonable first step would be to have a statement staying some attributes apply element-wise, let's say all that apply only to pointer types.
Afterwards, typeIncompatible can be adjusted easily. It's important that it's not only align though.

[dexonsmith]

llvm/test/Bitcode/attributes-3.3.ll

105

Hmm, I'm just seeing this now -- but attributes-3.3.ll.bc should not have been changed. Can that part of the change be reverted? Instead, the bitcode reader should have been adjusted to auto-upgrade the bitcode and strip the align attribute where appropriate.

Note that eventually the problem hit in production and @steven_wu added the bitcode upgrade in https://reviews.llvm.org/D102201. The whole reason we have these bitcode tests is to catch upgrade issues before commit.

[jdoerfert]

llvm/test/Bitcode/attributes-3.3.ll

105

With D102201 landed we can revert attributes-3.3.ll.bc. It makes sense to do it that way.

[dexonsmith]

llvm/test/Bitcode/attributes-3.3.ll

105

Huh; it looks like D102201 is insufficient, since it only fixes return types. attributes-3.3.ll.bc from 0a8e12fdbbf54e81954b091ac9fb11cd5f5148e1 has an attribute on parameter.

Would you please add the upgrade for parameters? I'm happy to review.

[steven_wu]

The reason I only fix return type in D102201 is that I identify the source of such incompatibility, which is coming from an optimization pass, so that can be wild spread incompatibility. The optimization pass explicitly happens when it is return type and check to remove all attributes that is incompatibly returned from typeIncompatible.

Fixing attribute on parameter will be nice but on the other hand, I am not sure if there is bitcode is the wild that consist of such incompatibility.

[jdoerfert]

In D87304#2764574, @steven_wu wrote:

The reason I only fix return type in D102201 is that I identify the source of such incompatibility, which is coming from an optimization pass, so that can be wild spread incompatibility. The optimization pass explicitly happens when it is return type and check to remove all attributes that is incompatibly returned from typeIncompatible.

Fixing attribute on parameter will be nice but on the other hand, I am not sure if there is bitcode is the wild that consist of such incompatibility.

I can imagine some IPOs to produce such arguments. I'll try to get to it. @steven_wu, if you want to just also do the argument case that would be appreciated.

llvm/lib/IR/Attributes.cpp

Incompatible.addAttribute(Attribute::SExt)
       .addAttribute(Attribute::ZExt);
 
   if (!Ty->isPointerTy())
     // Attribute that only apply to pointers.
     Incompatible.addAttribute(Attribute::Nest)
         .addAttribute(Attribute::NoAlias)
         .addAttribute(Attribute::NoCapture)
         .addAttribute(Attribute::NonNull)
+        .addAlignmentAttr(1)             // the int here is ignored
         .addDereferenceableAttr(1)       // the int here is ignored
         .addDereferenceableOrNullAttr(1) // the int here is ignored
         .addAttribute(Attribute::ReadNone)
         .addAttribute(Attribute::ReadOnly)
         .addAttribute(Attribute::StructRet)
         .addAttribute(Attribute::InAlloca)
         .addPreallocatedAttr(Ty)
         .addByValAttr(Ty)

llvm/test/Bitcode/attributes-3.3.ll

 }
 
 define void @f16() sspreq
 ; CHECK: define void @f16() #8
 {
         ret void;
 }
 
-define void @f17(i8 align 4 %0)
-; CHECK: define void @f17(i8 align 4 %0)
+define void @f17(i8* align 4 %0)
+; CHECK: define void @f17(i8* align 4 %0)
 {
         ret void;
 }
 
 define void @f18(i8* nocapture %0)
 ; CHECK: define void @f18(i8* nocapture %0)
 {
         ret void;

llvm/test/Bitcode/attributes.ll

 }
 
 define void @f16() sspreq
 ; CHECK: define void @f16() #8
 {
         ret void;
 }
 
-define void @f17(i8 align 4 %0)
-; CHECK: define void @f17(i8 align 4 %0)
+define void @f17(i8* align 4 %0)
+; CHECK: define void @f17(i8* align 4 %0)
 {
         ret void;
 }
 
 define void @f18(i8* nocapture %0)
 ; CHECK: define void @f18(i8* nocapture %0)
 {
         ret void;

llvm/test/Transforms/DeadArgElim/returned.ll

 ; don't do this if 'returned' is on the call site...
 ; CHECK-LABEL: define internal void @test5(%Ty* %this)
 
 define internal %Ty* @test5(%Ty* %this) {
   store volatile %Ty* %this, %Ty** @dummy
   ret %Ty* %this
 }
 
+; Drop all these attributes
+; CHECK-LABEL: define internal void @test6
+define internal align 8 dereferenceable_or_null(2) noalias i8* @test6() {
+  ret i8* null
+}
+
 define %Ty* @caller(%Ty* %this) {
   %1 = call %Ty* @test1(%Ty* %this)
   %2 = call %Ty* @test2(%Ty* %this)
   %3 = call %Ty* @test3(%Ty* %this)
   %4 = call %Ty* @test4(%Ty* %this)
 ; ...instead, drop 'returned' form the call site
 ; CHECK: call void @test5(%Ty* %this)
   %5 = call %Ty* @test5(%Ty* returned %this)
+  %6 = call i8* @test6()
   ret %Ty* %this
 }

llvm/test/Transforms/LoopSimplify/unreachable-loop-pred.ll

 
 foo:
   br label %while.body115
 }
 
 ; When loopsimplify generates dedicated exit block for blocks that are landing
 ; pads (i.e. innerLoopExit in this test), we should not get confused with the
 ; unreachable pred (unreachableB) to innerLoopExit.
-define align 8 void @baz(i32 %trip) personality i32* ()* @wobble {
+define void @baz(i32 %trip) personality i32* ()* @wobble {
 entry:
   br label %outerHeader
 
 outerHeader:
   invoke void @foo() 
           to label %innerPreheader unwind label %innerLoopExit
 
 innerPreheader:

llvm/test/Verifier/align.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+
+; CHECK: Wrong types for attribute: inalloca nest noalias nocapture nonnull readnone readonly byref(i32) byval(i32) preallocated(i32) sret(i32) align 1 dereferenceable(1) dereferenceable_or_null(1)
+; CHECK-NEXT: @align_non_pointer1
+define void @align_non_pointer1(i32 align 4 %a) {
+  ret void
+}
+
+; CHECK: Wrong types for attribute: inalloca nest noalias nocapture nonnull readnone readonly signext zeroext byref(void) byval(void) preallocated(void) sret(void) align 1 dereferenceable(1) dereferenceable_or_null(1)
+; CHECK-NEXT: @align_non_pointer2
+define align 4 void @align_non_pointer2(i32 %a) {
+  ret void
+}

llvm/test/Verifier/byref.ll

 }
 
 ; CHECK: Attributes 'byval', 'inalloca', 'preallocated', 'inreg', 'nest', 'byref', and 'sret' are incompatible!
 ; CHECK-NEXT: void (i32*)* @byref_nest
 define void @byref_nest(i32* byref(i32) nest) {
   ret void
 }
 
-; CHECK: Wrong types for attribute: inalloca nest noalias nocapture nonnull readnone readonly sret byref(i32) byval(i32) preallocated(i32) dereferenceable(1) dereferenceable_or_null(1)
+; CHECK: Wrong types for attribute: inalloca nest noalias nocapture nonnull readnone readonly sret byref(i32) byval(i32) preallocated(i32) align 1 dereferenceable(1) dereferenceable_or_null(1)
 ; CHECK-NEXT: void (i32)* @byref_non_pointer
 define void @byref_non_pointer(i32 byref(i32)) {
   ret void
 }
 
 define void @byref_callee([64 x i8]* byref([64 x i8])) {
   ret void
 }