Allow serialisation of call arguments with safe align attributes. #213
Conversation
This change permits the yk IR serialiser to serialise calls whose arguments use safe `align` attributes. Following the precedent of loads and stores, a safe alignment is one that is at least the size of the object being aligned. The JIT's code generator doesn't need to do anything special with such alignments. (We do plan to change our strategy and encode the alignment requirements explicitly into the IR and defer what to do with it to the JIT codegen, but now isn't the time) This allows us to handle calls like this: call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 8 dereferenceable(48) %3568, ptr noundef nonnull align 8 dereferenceable(48) %3567, i64 48, i1 false This one was found in the wild in the LuaJIT test suite.
| @@ -699,6 +699,19 @@ class YkIRWriter { | |||
| if (Attr.getKindAsEnum() == Attribute::AllocSize) { | |||
| continue; | |||
| } | |||
|
|
|||
| if (Attr.getKindAsEnum() == Attribute::Alignment) { | |||
| // Following what we do with loads/stores, we accept any alignment | |||
There was a problem hiding this comment.
This is problematic: we don't deal correctly with alignment bigger than the size of the object; we only deal correctly with equal to the size of the object AFAIK. So I think the line below (and loads/stores) should be = rather than >=.
There was a problem hiding this comment.
My understanding is that we musn't straddle an alignment boundary, which can't happen as long as the size of the object isn't bigger than the alignment. This came from a discussion with LLVM devs a few months back and ultimately lead to this comment explaining in the serialisation of loads/stores.
This section of the LLVM language reference talks about it too.
If the >= is incorrect (and you explain it to me) then we can tighten what we let through.
There was a problem hiding this comment.
Fundamentally we don't do anything with alignment properly :) The problem is that >= is very relaxed, and so it will be letting through even more things that we don't handle properly. Currently, from memory, we just align everything to the object size: so we do handle an object of n bytes if it's aligned to n bytes (which is the most common thing). But, say, we have an object of n bytes and it wants alignment 2n -- we'll still align it to n.
There was a problem hiding this comment.
OK. On Monday I'll change the serialiser to reject all non-natural alignments.
There was a problem hiding this comment.
Thanks! It would be good to catch these now, but I wonder if we've already been letting some through?!
There was a problem hiding this comment.
it can be done in one read without fear of straddling an alignment boundary, since the alignment is greater than the size of the object
I've just found a sentence in the LLVM language reference that explains what I'm trying to convey here:
An alignment value higher than the size of the loaded type implies memory up to the alignment value bytes can be safely loaded without trapping in the default address space.
There was a problem hiding this comment.
There are two kinds of memory location we have to consider:
Memory allocated, and therefore aligned by, the AOT compiler.- Memory allocated by JITted code.
Right, good point. A question is: how do we know we're doing the right thing with memory allocated by the JIT -- which includes things spilled to the stack? I'm not on top form today, so I'm struggling to think through the consequences here.
There was a problem hiding this comment.
A question is: how do we know we're doing the right thing with memory allocated by the JIT -- which includes things spilled to the stack?
It's a good question.
We've already established that for now the JIT isn't allocating any stack space with alloca instructions, so we can forget that (for now).
For malloc'd memory is allocated by an external facility, so I think we can ignore this as long as we abide by the load/store align requirements upon those pointers.
The spill case is one I hadn't considered and I had to think hard.
-
A llvm local variable can't have its address taken after the fact. If you need the address of something local to a frame, you'd have to
allocait and you'd get a pointer in return. Theallocaof,loads from andstores to this pointer would communicate alignment requirements explicitly at the IR level, but crucially the register allocator isn't "managing" the memory behind the alloca. -
A non-alloca'd local variable (a.k.a. an SSA variable) and has no requirement to live in memory at all. If a local variable spills in JITted code, then the only consumer of the spilled memory should be the unspill code of the JIT's register allocator itself. The allocator would be the only party blessed with the knowledge that a local variable spilled. So the spill/unspill code in the JIT's RA is free to mandate its own alignment requirements separate to those of
allocamemory in the LLVM IR and it (as far as I can see) has no need to communicate what they were to the wider world.
Do you buy this reasoning?
There was a problem hiding this comment.
That does sound plausible, so let's go with it. What's the right thing to do with this PR now?
There was a problem hiding this comment.
I think we've convinced ourselves that non-natural alignments where the alignment requirement is greater than or equal to the size of the object are OK. If that's the case the last two commits can be reverted. Is that OK?
(Reflection: For me this conversation has really highlighted that explicit alignment in the IR would be much easier to reason about. If for nothing else, because the locality of reasoning can be fully-contained in the JIT codegen -- right now you have to think about what the serialiser allows and how those properties flow through the IRs to the codegen)
|
Please force push an update. |
|
Force pushed. Cheers. |
This change permits the yk IR serialiser to serialise calls whose arguments use safe
alignattributes.Following the precedent of loads and stores, a safe alignment is one that is at least the size of the object being aligned. The JIT's code generator doesn't need to do anything special with such alignments.
(We do plan to change our strategy and encode the alignment requirements explicitly into the IR and defer what to do with it to the JIT codegen, but now isn't the time)
This allows us to handle calls like this:
This one was found in the wild in the LuaJIT test suite.