Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom alignment support, progress #11077

Closed
wants to merge 14 commits into from
Closed

Conversation

cooldome
Copy link
Member

align pragma now affects codegen. Align also implied packed, such that bigger alignment increase object size, but smaller alignment decrease it.

Summary of the changes:

  • size can now be specified for imported objects using size pragma
  • tfPacked type flag is superseded by tfUserAligment flag. Effectively packed pragma is equivalent of align: 1 pragma now.
  • alignment is restricted from 1 to 64 bytes to make is easier for allocator to pick it up in the feature.
  • runtime type information now contains alignment and it is ready to be picked up by allocator and GC
  • Overaligned objects are always passed by reference.

The feature remains widely undocumented for the main reason: allocator and GC now need to take alignment type information into account to make this feature truly public and useful.

Copy link
Collaborator

@mratsim mratsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work, I'm eagerly waiting for the aligned allocator as well :).

Nitpick, I'd like an additional test case with sizeof(T) to make sure it's usable with generics.

c: char
x: float

MyAlignedGenericType[T] {.align: 32.} = object
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a {.align: sizeof(T).} test as well?

@krux02
Copy link
Contributor

krux02 commented Apr 22, 2019

Sorry, but this is incorrect. User alignment cannot "superseed" the packed pragma. User alignment and packed are two different things. User alignment should affect the amount of padding bytes at the end of an object type, but it should not implicitly remove all padding bytes between its members.

type 
  MyAlignedType {.align: 64.} =
    someByte: byte
    someFloat: float64

doAssert(sizeof(MyAlignedType) == 64)
doAssert(offsetof(MyAlignedType,someFloat) == 8) # <- this will fail

Your implementation implicitly sets MyAlignedType to packed and the offset of someFloat will be 1 insead of 8.

Apart from this technical detail, I do agree that we need user alignment. But you need to extend your test cases to test the offset values of the object members as well.

@cooldome
Copy link
Member Author

@krux02: Thanks for review.

The actual problem that didn't know the difference between computeObjectOffsetsFoldFunction and computePackedObjectOffsetsFoldFunction so I used it blindly. It will not be hard to fix.

@mratsim
Copy link
Collaborator

mratsim commented Apr 23, 2019

This is related to this medium priority issue opened since 2015 #1930.
The issue asks for object fields alignment and not whole object alignment though.
This can be added later though with something like

type Foo = object
  x: int
  y{.align: 16.}: array[4, float32]

@krux02
Copy link
Contributor

krux02 commented Apr 23, 2019

computeObjectOffsetsFoldFunction is the default function that will be used to calculate the offsets of object (struct) members. The offset of each object member will be aligned to it's idividual alignment value. The alignment of the container object will be the maximum of all member alignment values. This alignment will also define the amount of padding bytes at the end of the object. computePackedObjectOffsetsFoldFunction is the simpler version of it that ignores alignement values of members for packed objects and sets the alignment of the object type to 1.

It would make sense if a compilation error would be risen if the programmer requests a low alignment value, for example 4, but one of the members requires a higher alignment value, for example 8.

type
  MyType {.align: 4.} = object
    member: float64  # <- should be a compilation error

Then, the last point is about packed objects with SIMD type members. Normally, when you have a packed object with unaligned members, everything works fine, except that performance might be bad. But for SIMD types this is a bit different. When you have non aligned SIMD types and try to load them, the program might even crash. So maybe it is worth to check if the programmer tries to do that and then raise a compilation error. But I am not sure if it is worth to implement the error checking right now, as the programmer will notice the error. It is just that the error could be prevented from the Nim compiler.

@cooldome
Copy link
Member Author

cooldome commented Apr 24, 2019

@krux02
At first I wanted make {.align: small_int.} to automatically pack with specified alignment. But looks like I couldn't make C compiler to do what I wanted it do.
So I will raise error just like you proposed.

Regarding packed pragma.
Packed type flag needs to be kept separate. I have tried with gcc compiler. Gcc refuses to pack simd types and keeps them aligned. I probably do the same if user alignment specified, packed pragma will keep user alignment. Need to try with vcc if it is the case as well.

@krux02
Copy link
Contributor

krux02 commented Apr 25, 2019

@cooldome

Regarding packed object with SIMD type members. I think it is best to introduce another flag for hard constraint alignment that would be used on SIMD types. Then I would raise a compilation error when somebody tries to pack an object with SIMD type members. This is really the safe way to do it, as you don't need to see what different C compilers do. Pseudocode:

type
  Vec4f {.align: 16, hardAlign.} = object
    data: array[4,float32]

  MyType {.packed.} = object
    a: byte
    v: Vec4f  # error packed pragma violates hard alignment constraint of Vec4f (align: 16)

@Araq
Copy link
Member

Araq commented Nov 15, 2019

Seems not to be required anymore now that we have fieldwise alignment annotations.

@Araq Araq closed this Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants