limit maximum vector alignment to heap alignment #21980

vchuravy · 2017-05-20T08:47:27Z

This imposes an upper limit on the vector alignment of 16. This is the current guaranteed alignment of the heap and after #21959 we can raise the limit to 64. In 0.5 we were a lot more conservative with the alignment given to memory loads.

I consider this a band-aid fix for #20961 and #21918. I don't know how the full fix for #20961 would look like, but I think it will involve fixing the DataLayout fallback in LLVM [1] and jl_special_vector_alignment will have to start taking the DataLayout into account.

[1] https://github.com/llvm-mirror/llvm/blob/836dd8e1f01318ac7f1149d399ce36b064404cb4/lib/IR/DataLayout.cpp#L507-L514

vchuravy · 2017-05-20T09:05:35Z

src/cgutils.cpp

@@ -524,7 +524,8 @@ static Type *julia_struct_to_llvm(jl_value_t *jt, jl_unionall_t *ua, bool *isbox
 #endif
                unsigned llvm_alignment = DL.getABITypeAlignment((Type*)jst->struct_decl);
                unsigned julia_alignment = jst->layout->alignment;
-                assert(llvm_alignment == julia_alignment);
+                assert(julia_alignment <= JL_SMALL_BYTE_ALIGNMENT &&
+                       julia_alignment == llvm_alignment);


This assert is wrong needs to be:

assert(julia_alignment <= JL_SMALL_BYTE_ALIGNMENT); assert(llvm_alignment <= JL_SMALL_BYTE_ALIGNMENT & julia_alignment == llvm_alignment);

vchuravy · 2017-05-21T09:37:38Z

@yuyichao should I split the last three commits out into a separate PR? This became more invasive then I thought.

vchuravy · 2017-05-21T11:16:41Z

@jlbuild

jlbuild · 2017-05-21T11:16:44Z

Status of `f993102` builds:

Builder Name	Build	Download
linux32	ERRORED	N/A
linux64	ERRORED	N/A
linuxaarch64	ERRORED	N/A
linuxarmv7l	ERRORED	N/A
linuxppc64le	ERRORED	N/A
osx64	ERRORED	N/A
win32	CANCELED	N/A
win64	ERRORED	N/A

vchuravy · 2017-05-21T11:17:12Z

@nanosoldier runbenchmarks(ALL, vs = ":master")

yuyichao · 2017-05-21T12:35:16Z

src/cgutils.cpp

+                unsigned julia_alignment = jl_datatype_align(jst);
+                // Check that the alignment adheres to the heap alignment.
+                assert(julia_alignment <= JL_HEAP_ALIGNMENT);
+                if (llvm_alignment  <= JL_HEAP_ALIGNMENT)


This should only be a temporary workaround.

yuyichao · 2017-05-21T12:37:24Z

src/gc.c

@@ -2832,15 +2833,15 @@ void *jl_gc_perm_alloc_nolock(size_t sz, int zero)
    if (__unlikely(sz > GC_PERM_POOL_LIMIT))
 #endif
        return zero ? calloc(1, sz) : malloc(sz);
-    sz = LLT_ALIGN(sz, JL_SMALL_BYTE_ALIGNMENT);
+    sz = LLT_ALIGN(sz, JL_HEAP_ALIGNMENT);


This shouldn't be changed. (i.e. this should stay at 16 when you change the heap_alignment to 64)

yuyichao · 2017-05-21T12:38:39Z

src/intrinsics.cpp

@@ -331,8 +331,7 @@ static Value *emit_unbox(Type *to, const jl_cgval_t &x, jl_value_t *jt, Value *d

    int alignment;
    if (x.isboxed) {
-         // julia's gc gives 16-byte aligned addresses
-        alignment = 16;
+        alignment = JL_HEAP_ALIGNMENT;


This shouldn't be changed since this won't increase. It might even be too much and should be sizeof(void*) * 2 now.

so it should be MAX_ALIGN?

yuyichao · 2017-05-21T12:48:09Z

src/cgutils.cpp

    }
+    if (alignment > JL_HEAP_ALIGNMENT)


This should be an assertion in the condition above. When used in pointerref/pointerset the user should be allowed to specify larger alignment.

yuyichao · 2017-05-21T12:54:22Z

I was reviewing under the assumption that JL_HEAP_ALIGNMENT is meant to be the max alignment that the GC can provide.

vchuravy · 2017-05-21T13:43:38Z

@jlbuild !nuke

jlbuild · 2017-05-21T13:43:40Z

Status of `151bd81` builds:

Builder Name	Nuke	Build	Download
linux32	COMPLETE	COMPLETE	Download
linux64	COMPLETE	COMPLETE	Download
linuxaarch64	COMPLETE	COMPLETE	Download
linuxarmv7l	COMPLETE	COMPLETE	Download
linuxppc64le	COMPLETE	COMPLETE	Download
osx64	COMPLETE	COMPLETE	Download
win32	COMPLETE	ERRORED	N/A
win64	COMPLETE	ERRORED	N/A

yuyichao · 2017-05-21T14:08:02Z

src/cgutils.cpp

-                assert(llvm_alignment == julia_alignment);
+                unsigned julia_alignment = jl_datatype_align(jst);
+                // Check that the alignment adheres to the heap alignment.
+                // TODO: Fix alignment calculation in LLVM.


Also fix the calculation on our side both in GC and in the struct declaration....

yuyichao · 2017-05-21T14:09:45Z

src/gc.c

@@ -2836,11 +2837,11 @@ void *jl_gc_perm_alloc_nolock(size_t sz, int zero)
    if (__unlikely(sz > gc_perm_size)) {
 #ifdef _OS_WINDOWS_
        void *pool = VirtualAlloc(NULL,
-                                  GC_PERM_POOL_SIZE + JL_SMALL_BYTE_ALIGNMENT,
+                                  GC_PERM_POOL_SIZE + JL_HEAP_ALIGNMENT,


..... The comment above applys to these too....... The permgen allocation shouldn't be changed.

yuyichao · 2017-05-21T14:10:07Z

src/intrinsics.cpp

@@ -331,7 +331,6 @@ static Value *emit_unbox(Type *to, const jl_cgval_t &x, jl_value_t *jt, Value *d

    int alignment;
    if (x.isboxed) {
-         // julia's gc gives 16-byte aligned addresses


Keep this comment for now I guess...

nanosoldier · 2017-05-21T14:10:47Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

jlbuild · 2017-05-22T00:47:00Z

Status of `3c39b0a` builds:

Builder Name	Nuke	Build	Download	Code Output
linux32	COMPLETE	COMPLETE	Download	ERRORED
linux64	COMPLETE	COMPLETE	Download	COMPLETE
linuxaarch64	COMPLETE	COMPLETE	Download	COMPLETE
linuxarmv7l	COMPLETE	COMPLETE	Download	COMPLETE
linuxppc64le	COMPLETE	COMPLETE	Download	COMPLETE
osx64	COMPLETE	COMPLETE	Download	PENDING

vchuravy · 2017-05-22T13:49:15Z

@eschnett have you seen the SIMD.jl failure on 32bit linux before? https://buildog.julialang.org/#/builders/45/builds/1

Test Failed
  Expression: s === sum(xs)
   Evaluated: -3.927754334663871e235 === 91.0
ERROR: LoadError: There was an error during testing
while loading /home/buildworker/.julia/v0.7/SIMD/test/runtests.jl, in expression starting on line 372

tkelman · 2017-05-22T16:41:07Z

test/vecelement.jl

+    return quote
+            Base.@_inline_meta
+            Base.llvmcall($exp,
+               NTuple{N, VecElement{T}},


unusual indent in this block?

tkelman · 2017-05-22T16:42:48Z

test/vecelement.jl

+        b = f20961([a], [a])
+        @test b == result
+        if N == 36
+            code_llvm(STDOUT, f20961, (Vector{Vec{N, T}}, Vector{Vec{N, T}}))


is this temporary? less noisy to output this to a buffer and test for something instead?

Make julia_alignment actually return the jl_datatype_align if `alignment == 0` and check that the requested alignment is coherent with the heap alignment.

vchuravy · 2017-05-23T21:26:47Z

If there are no further comments I plan to merge this in about 24h

(cherry picked from commit c2c0997) ref #21980 introduce jl_datatype_align (cherry picked from commit 5115a38) Introduce JL_HEAP_ALIGNMENT (cherry picked from commit ed286e4) fix julia_alignment Make julia_alignment actually return the jl_datatype_align if `alignment == 0` and check that the requested alignment is coherent with the heap alignment. (cherry picked from commit b9671b0)

vchuravy added backport pending 0.6 bugfix This change fixes an existing bug needs tests Unit tests are required for this change labels May 20, 2017

vchuravy requested review from eschnett, vtjnash and ArchRobison May 20, 2017 08:47

vchuravy commented May 20, 2017

View reviewed changes

vchuravy force-pushed the vc/vec_alignment branch from 68609f3 to 87ccf70 Compare May 21, 2017 08:03

vchuravy removed the needs tests Unit tests are required for this change label May 21, 2017

vchuravy requested a review from yuyichao May 21, 2017 11:18

yuyichao requested changes May 21, 2017

View reviewed changes

vchuravy force-pushed the vc/vec_alignment branch from 151bd81 to 3c39b0a Compare May 21, 2017 14:40

vchuravy changed the title ~~[RFC] limit maximum vector alignment to heap alignment~~ limit maximum vector alignment to heap alignment May 21, 2017

vchuravy requested a review from yuyichao May 22, 2017 13:47

yuyichao approved these changes May 22, 2017

View reviewed changes

tkelman reviewed May 22, 2017

View reviewed changes

vchuravy added 2 commits May 23, 2017 08:53

limit maximum vector alignment to heap alignment

c2c0997

introduce jl_datatype_align

5115a38

vchuravy added 2 commits May 23, 2017 08:53

Introduce JL_HEAP_ALIGNMENT

ed286e4

fix julia_alignment

b9671b0

Make julia_alignment actually return the jl_datatype_align if `alignment == 0` and check that the requested alignment is coherent with the heap alignment.

vchuravy force-pushed the vc/vec_alignment branch from 3c39b0a to b9671b0 Compare May 22, 2017 23:53

vchuravy merged commit 298fffb into master May 24, 2017

vchuravy deleted the vc/vec_alignment branch May 24, 2017 23:46

fredrikekre mentioned this pull request May 26, 2017

Segfault with longer SIMD vectors #20961

Closed

vchuravy mentioned this pull request May 26, 2017

Loading a NTuple{N, VecElement{T}} can seqfault #21918

Closed

KristofferC mentioned this pull request May 30, 2017

Triggers a Julia assertion on nightly Ferrite-FEM/Tensors.jl#62

Closed

tkelman removed the backport pending 0.6 label Jun 7, 2017

vtjnash mentioned this pull request Oct 19, 2019

Wrong GEP emission for tuple containing VecElement #32414

Closed

vtjnash mentioned this pull request Mar 11, 2024

vecelement test fails on Zen4 #53683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

limit maximum vector alignment to heap alignment #21980

limit maximum vector alignment to heap alignment #21980

vchuravy commented May 20, 2017

vchuravy May 20, 2017

vchuravy commented May 21, 2017

vchuravy commented May 21, 2017 •

edited

Loading

jlbuild commented May 21, 2017 •

edited

Loading

vchuravy commented May 21, 2017

yuyichao May 21, 2017

yuyichao May 21, 2017

yuyichao May 21, 2017

vchuravy May 21, 2017

yuyichao May 21, 2017

yuyichao commented May 21, 2017

vchuravy commented May 21, 2017

jlbuild commented May 21, 2017 •

edited

Loading

yuyichao May 21, 2017

yuyichao May 21, 2017

yuyichao May 21, 2017

nanosoldier commented May 21, 2017

jlbuild commented May 22, 2017 •

edited

Loading

vchuravy commented May 22, 2017

tkelman May 22, 2017

tkelman May 22, 2017

vchuravy commented May 23, 2017

limit maximum vector alignment to heap alignment #21980

limit maximum vector alignment to heap alignment #21980

Conversation

vchuravy commented May 20, 2017

Choose a reason for hiding this comment

vchuravy commented May 21, 2017

vchuravy commented May 21, 2017 • edited Loading

jlbuild commented May 21, 2017 • edited Loading

Status of f993102 builds:

vchuravy commented May 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuyichao commented May 21, 2017

vchuravy commented May 21, 2017

jlbuild commented May 21, 2017 • edited Loading

Status of 151bd81 builds:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanosoldier commented May 21, 2017

jlbuild commented May 22, 2017 • edited Loading

Status of 3c39b0a builds:

vchuravy commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchuravy commented May 23, 2017

vchuravy commented May 21, 2017 •

edited

Loading

jlbuild commented May 21, 2017 •

edited

Loading

Status of `f993102` builds:

jlbuild commented May 21, 2017 •

edited

Loading

Status of `151bd81` builds:

jlbuild commented May 22, 2017 •

edited

Loading

Status of `3c39b0a` builds: