Tweak successor lowering / filter out debug instructions from AOT IR. #124
Conversation
This is more resilient to changes in LLVM.
| // | ||
| // FIXME: I don't like this much: | ||
| // | ||
| // - Assumes one LLVM instruction becomes exactly one Yk IR instruction. |
There was a problem hiding this comment.
This seems like a dangerous assumption!
There was a problem hiding this comment.
It's annoying. I agree.
The problem is, LLVM's output streamer can only do exactly that: stream. We need to be able to emit an instruction count, and then the payload for the instructions. But at present we can't say how many instructions we will emit, without actually doing the lowering.
It just so happens that (aside from debug instructions now) it's currently a 1-1 mapping...
There was a problem hiding this comment.
It's because the IR will be streamed in later, and the reader needs to know how many to read. Such is serialisation.
We could use some kind of sentinel instead I suppose. If we steal an opcode, we could use that for "end of stream".
There was a problem hiding this comment.
If we don't know the size, then we can a) just buffer it until we do know the size b) put a dummy size and patch it later. Failing that an "EOS" thingy might be a good idea.
There was a problem hiding this comment.
Yes, just its just a larger change than I want to make now.
There was a problem hiding this comment.
(An llvm output streamer can't be rewound, so patching is probably out of the question)
There was a problem hiding this comment.
As a quick hack can we add a count of both LLVM and our IR and just assert at the end that the two are the same? If we ever mess things up, it'll remind us.
There was a problem hiding this comment.
4a22f6f kills the debug print I left in.
| // instrs: | ||
| unsigned InstIdx = 0; | ||
| for (Instruction &I : BB) { | ||
| if (ShouldSkipInstr(&I)) { |
There was a problem hiding this comment.
Doesn't this invalidate the "one LLVM instruction == one yk IR instruction" assumption?
There was a problem hiding this comment.
We are fine for now.
Before this change we counted the number of LLVM AOT instructions and assumed that we would see as many instructions in our AOT IR.
When I initially skipped the debug instructions, I broke the serialiser, because by skipping debug instructions, the count was too high, and I got UB fun at runtime.
The ShouldSkipInstr check ensures the count is valid again.
If later any instruction lowering emits either 0 or >1 Yk instruction, we will have to hack around that too.
I'd like it if we could serialise the instructions to a buffer and count as we go, but that's a bigger change that I don't want to get into now.
There was a problem hiding this comment.
All of this to say: I've not introduced any new badness, only hacked around existing badness :)
|
I'm ok with this. Does @ltratt need to check the new commits? Otherwise please squash. |
|
I believe I've done as he requested, so I'll squash. |
Also added a comment about the way we lower the AOT IR.
Before:
```
bb1:
call llvm.dbg.value(?op<i32 %0>, ?op<!31 = !DILocalVariable(name: "argc", arg: 1, scope: !23, file: !2, line: 46, type: !26)>, ?op<!DIExpression()>)
call llvm.dbg.value(?op<ptr %1>, ?op<!32 = !DILocalVariable(name: "argv", arg: 2, scope: !23, file: !2, line: 46, type: !27)>, ?op<!DIExpression()>)
$1_2: ptr = ptradd $0_1, 16i64
store $1_2, GlobalDecl(shadowstack_0, tls=false)
$1_4: ptr = call yk_mt_new(const_ptr)
call llvm.experimental.stackmap(1i64, 0i32, $0_0, $0_1, $0_4, $0_5, $0_6, $0_7)
br
```
After:
```
bb1:
$1_0: ptr = ptradd $0_1, 16i64
store $1_0, GlobalDecl(shadowstack_0, tls=false)
$1_2: ptr = call yk_mt_new(const_ptr)
call llvm.experimental.stackmap(1i64, 0i32, $0_0, $0_1, $0_4, $0_5, $0_6, $0_7)
br
```
|
squashed. |
See commits.