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.

Sign up for GitHub

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

Allow serialisation of call arguments with safe align attributes. #213

Merged
merged 1 commit into from
Nov 11, 2024
Merged

Allow serialisation of call arguments with safe align attributes. #213

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions llvm/lib/YkIR/YkIRWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

OK. On Monday I'll change the serialiser to reject all non-natural alignments.

Copy link

Choose a reason for hiding this comment

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

Thanks! It would be good to catch these now, but I wonder if we've already been letting some through?!

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 alloca it and you'd get a pointer in return. The alloca of, loads from and stores 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 alloca memory 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?

Copy link

Choose a reason for hiding this comment

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

That does sound plausible, so let's go with it. What's the right thing to do with this PR now?

Copy link
Author

Choose a reason for hiding this comment

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

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)

// value greater-than or equal-to the size of the object.
//
// FIXME: explicitly encode the alignment requirements into the IR
// and let the JIT codegen deal with it.
if (I->getParamAlign(AI) >=
DL.getTypeAllocSize(I->getArgOperand(AI)->getType())) {
continue;
}
}

serialiseUnimplementedInstruction(I, FLCtxt, BBIdx, InstIdx);
return;
}
Expand Down