PATCHABLE_* instructions expand to up to 36-byte
sleds. Updating the size of PATCHABLE instructions
causes them to be outlined, so we need to add a
check to prevent the outliner from considering
basic blocks that contain PATCHABLE instructions.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/lib/CodeGen/TargetInstrInfo.cpp | ||
|---|---|---|
| 1653–1654 | I think this could be a llvm::find_if? return find_if(MBB.getFirstNonDebugInstr(), MBB.getLastNonDebugInstr(), [](const MachineInstr &MI) {
unsigned Opc = MI.getOpcode();
switch(Opc) {
default:
return false;
case TargetOpcode::FENTRY_CALL:
...
return true;
}
}) != MBB.end();That'd make it easy to add new opcodes/remove opcodes. | |
| llvm/lib/CodeGen/TargetInstrInfo.cpp | ||
|---|---|---|
| 1653–1654 | It definitely could be, and it makes the function visually a lot cleaner. Because PATCHABLE instructions are always inserted at the start or at/just before the end, the previous and current implementations are O(1) in the number of instructions. Being fairly unfamiliar with the performance characteristics of the codebase, I was hesitant to scan through the whole basic block. If the small performance difference isn't a concern, I'd be happy to switch to llvm::find_if. | |
| llvm/lib/CodeGen/TargetInstrInfo.cpp | ||
|---|---|---|
| 1653–1654 | Ah I see. Yeah let's keep it like this. | |
Comment Actions
I think it would be good to have a MIR testcase for this.
Other than that, I think this looks good.
Comment Actions
branch-relax-xray.ll tests that Branch Relaxation relaxes branches that are out-of-range because of XRay sleds, and machine-outliner-patchable.ll tests that XRay sleds aren't outlined. Why the MIR test case?
Comment Actions
Because we can see clearly which MIR opcodes should not be outlined, and it defends us against other optimizer changes in earlier passes. Like, if the MIR test starts failing, we know 100% that it's an outliner issue and not some other pass dropping the X-ray ops or something.
Comment Actions
Test to make sure XRay patchable instructions aren't outlined. I think technically I could break this into two separate patches now that I have two independent tests. I don't think that's necessary, but please let me know if it's preferred.
Comment Actions
I think it's okay to keep all of this in one patch.
I put the MIR test through Compiler Explorer to see what is outlined (and removed some of the stuff that isn't necessary in MIR):
https://godbolt.org/z/e85n9Wh8r
It looks like this, without this patch, does outline something with PATCHABLE_FUNCTION_EXIT, but we don't get any outlining behaviour for PATCHABLE_FUNCTION_ENTRY.
Would it be possible to add a case where PATCHABLE_FUNCTION_ENTRY is outlined without this patch?
Comment Actions
Because isMBBSafeToOutlineFrom already accounted for TargetOpcode::FENTRY_CALL and TargetOpcode::PATCHABLE_FUNCTION_ENTER, I won't be able to write a test that makes PATCHABLE_FUNCTION_ENTER outlined without this patch.
I could probably write a test that makes sure TargetOpcode::PATCHABLE_RET and TargetOpcode::PATCHABLE_TAIL_CALL aren't outlined, but neither of those instructions are inserted on AArch64 (they're only inserted on ppc64le and x86_64, respectively). AFAIK no test exists to make sure they aren't outlined.
I think this could be a llvm::find_if?
return find_if(MBB.getFirstNonDebugInstr(), MBB.getLastNonDebugInstr(), [](const MachineInstr &MI) { unsigned Opc = MI.getOpcode(); switch(Opc) { default: return false; case TargetOpcode::FENTRY_CALL: ... return true; } }) != MBB.end();That'd make it easy to add new opcodes/remove opcodes.