Conversation
1cfe028 to
46b0d49
Compare
|
This patch begins a more comprehensive take on #5279, which handles a key edge cases with proper alignment tracking. LDC has a serious case of forgetting alignment throughout the flow. There's another one after this which similarly treads some other territory. |
|
Thanks, that looks better - and storing the alignment in the DLValue makes sense to me.
Cool, thx for tracking down and fixing that one. |
| const auto minAlignment = | ||
| std::min(DtoAlignment(val->type), static_cast<unsigned>(alignment)); | ||
| DtoMemCpy(copy, lval, DtoConstSize_t(minSize), minAlignment); | ||
| DtoMemCpy(copy, lval, DtoConstSize_t(minSize), minAlignment, minAlignment); |
There was a problem hiding this comment.
alignment is the destination alignment, val's (new) the source one - no need for minAlignment anymore due to the split alignments now.
| "made to."); | ||
| return new DLValue(type, getIrValue(vd)); | ||
| return new DLValue(type, getIrValue(vd), | ||
| vd->isReference() ? DtoAlignment(type) : 1); |
There was a problem hiding this comment.
DtoAlignment(type) should be fine for non-refs too, propagating the alignment of the alloca.
| src = indexVThis(ad, thisptr); | ||
| } | ||
| if (depth > 1) { | ||
| const unsigned ptrAlign = getABITypeAlign(getOpaquePtrType()); |
There was a problem hiding this comment.
IIRC, we use target.ptrsize as pointer alignment in other places already.
| if (retValIsLVal) { | ||
| return new DLValue(resulttype, retllval); | ||
| return new DLValue(resulttype, retllval, | ||
| tf->isRef() ? DtoAlignment(resulttype) : 1); |
There was a problem hiding this comment.
DtoAlignment(resulttype) should be fine for non-refs too (in which case - retValIsLVal - the lvalue has been allocated by the caller on its stack, and that alloca has the type alignment).
There was a problem hiding this comment.
retllval can be in-place construction's sret, for a field which may be align(1). I have a follow-up patch coming which works through a broader set of cases though, and removes this again with that.
| LLValue *v = p->func()->thisArg; | ||
| result = new DLValue(e->type, v); | ||
| const bool isStruct = e->type->toBasetype()->ty == TY::Tstruct; | ||
| result = new DLValue(e->type, v, isStruct ? DtoAlignment(e->type) : 1); |
There was a problem hiding this comment.
For classes, it's the ClassDeclaration::alignsize IIRC.
There was a problem hiding this comment.
For classes this lvalue is the stack slot holding this, so it's pointer-aligned. alignsize applies to field accesses through the reference; that comes in the follow-up I mentioned above, and it needs dmd #23966... which was my next question; how do we make a change to LDC which depends on a DMD update?
| LLValue *initsym = getIrAggr(sd)->getInitSymbol(); | ||
| assert(dstMem->getType() == initsym->getType()); | ||
| DtoMemCpy(DtoType(e->type), dstMem, initsym); | ||
| DtoMemCpy(DtoType(e->type), dstMem, initsym, false, dstAlign); |
There was a problem hiding this comment.
The init symbol's alignment is the type's.
| static DLValue *emitStructLiteral(StructLiteralExp *e, | ||
| LLValue *dstMem = nullptr) { | ||
| LLValue *dstMem = nullptr, | ||
| unsigned dstAlign = 1) { |
There was a problem hiding this comment.
Please no default value here, this is an internal helper called twice, so the call sites should take care of passing an alignment.
Struct assignment and struct zero/static initialisation emitted llvm.memcpy/llvm.memset with alignment 1, so LLVM had to treat every struct copy as potentially unaligned whenever it could not see where the pointer came from. On strict-alignment targets that lowers each copy to byte loads and stores. DLValue now records the alignment its pointer is guaranteed to have, defaulting to 1 (unknown). Lvalues formed by dereferencing a pointer (*p, p[i], slice[i]), ref returns, ref locals and the `this` of struct methods take the pointee type's alignment, which LDC already assumes for every scalar load and store through such pointers. Parameters and init symbols take their type's alignment, as their storage is allocated for it. Struct copies and zero-initialisations use the recorded alignment of each side. Field addresses and static array elements stay at 1 for now; their alignment depends on the containing aggregate's alignment and the field offset.
46b0d49 to
1dc79c8
Compare
|
Fixed most of those things, 2 should stay as they are until the follow-up. How do we update DMD though? As soon as the alignments all flow properly, DMD reveals that bug. |
Struct assignment and struct zero/static initialisation emit
llvm.memcpy/llvm.memsetwith alignment 1. Wherever LLVM can't see where a pointer came from (anything reached through a pointer, slice orref), every struct copy is treated as potentially unaligned. On strict-alignment targets (e.g.-mtriple=armv5te-none-eabi -mattr=+strict-align) each copy then lowers to a run ofldrb/strb.This PR makes
DLValuerecord the alignment its pointer is guaranteed to have, defaulting to 1 (unknown), and sets it only where it follows from the pointer's type:*p,p[i],slice[i]refparameters,refreturns,reflocalsthisin struct methodsEach of these takes the pointee type's alignment (
DtoAlignment(T), soalign(1) structstays at 1 andalign(16) structgets 16). That's the assumption LDC already makes for every scalar load and store through such pointers. Struct copies use the recorded alignment of each side separately (DtoMemCpynow takes a destination and a source alignment), and zero/static initialisation uses the destination's.Not covered here: field addresses (
s.f,obj.f) and static array elements stay at 1. Their alignment depends on the containing aggregate's alignment and the field offset (MinAlign(base, offset)), and doing that properly means scalar loads and stores should honour it too. Today they always assume the ABI alignment, so auintfield in analign(1)struct is loaded withalign 4. I'll do that as a follow-up. That follow-up depends on dlang/dmd#23966:UnionExpemplacesRealExp/ComplexExpinto storage aligned to 8 whilerealneeds 16 on x86_64 posix, which an earlier version of this change (#5279) tripped over in the self-hosted build.Tested with the new
tests/codegen/struct_copy_alignment.dand the lit suite. I also built ldc2 with the patched compiler and compiled Phobos'std/stdio.dunittests with it using the CI flags, the step where #5279 failed.