Add pass disallowing calls in entry blocks. #105
Conversation
|
Can we find a test for the stackmap fix? cvise might be our friend here? |
|
There's a test in yk that fails without this change. |
|
Understood, but we'll never be able to upstream this to LLVM without an LLVM-level test. Maybe we can just extract the |
|
That should be possible. |
|
Thanks! |
|
Sorry, I accidentally rebased locally whilst working on the test. Permission to force push before I add the test? |
llvm/lib/CodeGen/StackMaps.cpp
Outdated
| if (MOI->isKill()) { | ||
| // There's no point in tracking a killed register. Instead, try and | ||
| // replace it with another register with the same value (possibly due | ||
| // to spilling), to increase the likelyhood off us tracking that value. |
There was a problem hiding this comment.
We discussed this the other day, but I wouldn't say a temporary save/restore from one register to another is a "spill".
You said LLVM uses this term that way though? Is that so? Can you show us?
There was a problem hiding this comment.
Can't find it anymore and don't really care enough to keep looking. Happy for you to suggest a different wording if you dislike the term spilling to describe storing one register in another to free the first one up.
There was a problem hiding this comment.
you can just say the value was moved to another register. There are many reasons why that could happen, the "spill" being just one of them.
| @@ -99,13 +113,14 @@ void processInstructions( | |||
| const MachineOperand Rhs = Instr.getOperand(1); | |||
| assert(Lhs.isReg() && "Is register."); | |||
| assert(Rhs.isReg() && "Is register."); | |||
| // Reassigning a new value to Lhs means any mappings to Lhs are now void | |||
|
Just some superficial comments from me. Good work! |
If you rebase back to the last commit in this PR (357fc57ab34f2557a303ee90bbaa912da3472559) then no force pushing will be necessary. |
|
sigh ok, I just need to remember to move these changes 357fc57#diff-1d872ec9f8bee5a945bd90f3aa4e0190cbc47e55a0c0dec7d6f8cc92c3e9af66L82 from the second commit to the first, which I've already done, but which is gonna disappear when I reset to 357fc57 |
|
That should address all comments. Needs squashing and rebasing as I need to move something from commit 2 into commit 1. |
| // YKFIXME: This is likely only a temporary fix. In the future we might | ||
| // want to allow stackmaps to track arbitrarily many locations per live | ||
| // variable. Currently, we can only track two. | ||
| // YKFIXME: This is likely only a temporary fix. In the future we might |
There was a problem hiding this comment.
line 313: off -> of
I wonder if we can reword it a bit to be easier to read.
How about:
Instead, we look for copies of the killed register and use one of those registers instead.
There was a problem hiding this comment.
Fixed in 92fb504.
Review Bot: You have now depleted your free change requests. Please buy more review tokens if you like to request more changes.
|
Please squash. |
ptersilie
force-pushed
the
nomoreentryblockcalls
branch
from
92fb504 to
e4ee8de
Compare
|
Squashed an rebased. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
The renaming of the pass broke the formatting. Ok to squash? |
|
Please squash. |
The pass splits entry block at the first call instruction, thus ensuring that no entry block has a function call. This makes it easier during trace construction to determine when external functions call back into mappable code.
This commit fixes yet another bug in stackmaps that leads to registers being untracked. The bug went unnoticed until the recent entryblock change exposed it. The fix consists of two changes, allowing spill mappings to be applied transitively and replacing killed registers in the stackmap with non-killed registers holding the same value. See comments for more detail.
ptersilie
force-pushed
the
nomoreentryblockcalls
branch
from
4946fb6 to
10bf29b
Compare
|
Squashed. |
Adds a pass that splits the entry block before the first call instruction, thus ensuring that no calls can exist inside a function's entry block. This makes it easier on the JIT side to detect mappable callbacks from unmappable code (aka external functions) and fixes a soundness issue with our previous StackAdjust solution.
Unfortunately, this change revealed a stackmap bug that was lurking around, so we need to fix them along side this change.
yk companion PR coming up soon.