Page MenuHomePhabricator

[AttributeFuncs] Consider `align` in `typeIncompatible`
ClosedPublic

Authored by jdoerfert on Sep 8 2020, 11:18 AM.

Details

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.

Diff Detail

Event Timeline

jdoerfert created this revision.Sep 8 2020, 11:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2020, 11:18 AM
jdoerfert requested review of this revision.Sep 8 2020, 11:18 AM
jdoerfert updated this revision to Diff 290562.EditedSep 8 2020, 12:20 PM

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

fhahn added inline comments.Sep 9 2020, 8:49 AM
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 added inline comments.Sep 9 2020, 1:23 PM
llvm/test/Bitcode/attributes-3.3.ll
105

it was updated because of that, yes.

fhahn accepted this revision.Sep 10 2020, 6:18 AM

LGTM, thanks!

This revision is now accepted and ready to land.Sep 10 2020, 6:18 AM
This revision was automatically updated to reflect the committed changes.

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?

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?

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 added a subscriber: anna.Wed, Oct 14, 5:29 AM

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 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.