Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Context: this is part of the patch which fixes an issue seen when building rust-analyzer for riscv64 linux hosts. I think this patch also fixes an issue seen by the Zig toolchain developers, who were trying to compile the LLVM NVPTX backend for riscv64 linux hosts.
@msizanoen1 Please can you upload the entire patch that you proposed in the rust-lang thread (including the parts about reordering branch relaxation), and also ensure you include all the context using git diff -U999999 there are more instructions here: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
Thanks for getting the patch prepared so quickly!
| llvm/lib/Target/RISCV/RISCVTargetMachine.cpp | ||
|---|---|---|
| 35–37 ↗ | (On Diff #281515) | Do we really want to be able to turn this off? It's only used when needed, and I don't see why you'd ever want to force code to fail to compile that could easily be fixed up by the compiler to work. Do note that the relaxation pass does double-duty, doing *both* direct -> indirect relaxation (rare) but *also* conditional direct -> unconditional direct relaxation which isn't that rare given the 13-bit signed displacement available for conditional branches (compared with JAL's 21-bit signed displacement). For example, I know FreeBSD kernels hits the latter case in a few places. Though even if this option only affected the indirect relaxation I'd still argue your compiler shouldn't provide an option that does nothing to affect CodeGen other than artificially stopping it from compiling certain programs. |
| 182–184 ↗ | (On Diff #281515) | This should not matter. All the pseudoinstructions are supposed to have correct lengths for their expanded forms (see https://reviews.llvm.org/D77443), so please revert this hunk. That also means you can use PseudoLLA in insertIndirectBranch rather than having to duplicate the expansion of it. |
(the fact that FreeBSD kernel builds could trigger relaxation is what exposed the bug fixed in https://reviews.llvm.org/D77443)
Please avoid duplication. Something like this should be better in that regard, both for duplicating between the if/else branches and duplicating with the existing AUIPC-based expansions elsewhere.
| llvm/lib/Target/RISCV/RISCVInstrInfo.cpp | ||
|---|---|---|
| 398–419 | ||
Although I feel PseudoJump might be a sensible choice even for position-dependent code? By this point we know the branch target is more than 2 MiB away, and I assume that neither LUI/JALR nor AUIPC/JALR will be either relaxable at link time (branch target would need to be a low address) or compressible. Plus AUIPC/JALR is more like JAL in being PC-relative, so it "feels" more "faithful".
This still need to be backported to rust-lang/llvm-project fork which is still on LLVM 10 so using PseudoJump is not an option.
The Rust community is free to do what it likes in its fork, but their restrictions should not cause us as upstream to commit lower-quality code. There's also nothing stopping them back-porting https://reviews.llvm.org/D73178 (and https://reviews.llvm.org/D77443, which they'll need now too). So please, let's keep the code in LLVM upstream as clean and concise as possible.
| llvm/lib/Target/RISCV/RISCVInstrInfo.cpp | ||
|---|---|---|
| 398 | I don't know how much it matters, but we know the register is dead here. | |
This looks good to me now, let's see what others think. But please update llvm/test/CodeGen/RISCV/branch-relaxation.ll to reflect the change in CodeGen from LUI/JALR to AUIPC/JALR, and add PIC RUN lines. Plus whilst you're there I guess it wouldn't hurt to add RV64 variants too.
(and my ; TODO: Extend simm12's MCOperandPredicate so the jalr zero is printed as a jr. then goes away, not because that's been fixed, but because by using PseudoJump we no longer expose it... perhaps that comment should be relocated to RISCVInstrInfo.td's definition of simm12, but it's a minor stylistic thing, it doesn't really matter beyond not being the canonical alias)
| llvm/test/CodeGen/RISCV/branch-relaxation.ll | ||
|---|---|---|
| 5 ↗ | (On Diff #281582) | |
You've lost the check for the non-PIC one and didn't check that the PIC one actually assembles (it really should, but you never know; the point is that unless you emit a .o the fixups are never actually computed and thus you won't get errors if they end up overflowing).
| llvm/test/CodeGen/RISCV/branch-relaxation.ll | ||
|---|---|---|
| 2–3 ↗ | (On Diff #281585) | |
| 4 ↗ | (On Diff #281585) | |
| llvm/test/CodeGen/RISCV/branch-relaxation.ll | ||
|---|---|---|
| 4 ↗ | (On Diff #281592) | This isn't quite what I suggested, as it's too long a line without wrapping more of the arguments to the next line. |
@jrtc27 are you happy with this patch now? Thanks for your review on this and thanks @msizanoen1 for providing this fix.
I think @jrtc27's suggestion about deleting/moving the TODO line in test code still needs to be addressed.
Other than the TODO, yes (and hopefully you agree with my suggestions here!). I would also like to see branch-relaxation.ll grow RV64 RUN lines for completeness, even if it's probably a bit redundant (and creates a slight explosion..), but I can do that as a follow-up patch.
This patch still seems to need some additional work. When I apply it and run the tests I get a crash for the branch-relaxation.ll test:
*** Bad machine code: MBB has unexpected successors which are not branch targets, fallthrough, EHPads, or inlineasm_br targets. *** - function: relax_jal - basic block: %bb.3 (0x5621c4161a48)
Ah, that's because the TableGen definition is broken (works fine for asm, but not code gen):
let isCall = 0, isBarrier = 0, isCodeGenOnly = 0, hasSideEffects = 0,
mayStore = 0, mayLoad = 0 inI _believe_ we need:
isBranch = 1, isTerminator = 1, isBarrier = 1
?
Sounds about right. (I don't know offhand in what circumstances you have different values for isTerminator and isBarrier).
With that change we now run into an llvm_unreachable in RISCVInstrInfo::isBranchOffsetInRange, maybe it's just implementing that missing case.
Given @luismarques's comment, have you now actually run the tests (and I mean all RISC-V tests, not just branch-relaxation.ll, in case anything has been missed)? Perhaps I shouldn't take it for granted that people run tests before submitting patches, including revisions (though I'd still verify myself before committing anything), but you really should as otherwise it just wastes our time having to tell you they're broken when you could just run them yourself.
| llvm/lib/Target/RISCV/RISCVInstrInfo.td | ||
|---|---|---|
| 1015–1017 ↗ | (On Diff #281926) | Group the branch/terminator-related bits together and avoid unnecessary line breaks |
My guess would be barrier implies terminator but that you can't really express that in TableGen (though maybe clever use of ? and backend tweaks could do something ad-hoc) so for simplicity you just have to specify both. The other way round certainly doesn't hold; conditional branches can terminate a basic block and fall through to the next one.
With that change we now run into an llvm_unreachable in RISCVInstrInfo::isBranchOffsetInRange, maybe it's just implementing that missing case.
Yeah, looks like it. The easy answer is to do isIntN(32, BrOffset), though that's not strictly correct on RV64 due to the implicit + 0x800 in the HI/LO relocations, so should really be isIntN(32, SignExtend64(BrOffset + 0x800, XLen)) I think, and is basically what LLD does for checking the range of HI20 values (LLD also left-shifts and does an isInt<20> instead, but they're equivalent). The same also goes for the assertion in insertIndirectBranch. I won't ask why we use isInt<X>(...) there but isIntN(X, ...) in isBranchOffsetInRange... :)
| llvm/lib/Target/RISCV/RISCVInstrInfo.cpp | ||
|---|---|---|
| 428 | Isn't STI a RISCVSubtarget, so we can just call getXLen? | |
| 446 | Nowhere else in the tree casts for uses of SignExtend64 like this (e.g. MipsMCExpr.cpp), even if strictly there could be signed integer overflow here, but where you've put the cast is too late. If you're going to cast anywhere, do it on BrOffset itself, but I'd probably just match what everyone else does and drop it althogether. | |
| llvm/lib/Target/RISCV/RISCVInstrInfo.td | ||
| 1015–1017 ↗ | (On Diff #281926) | Please combine the two short lines like I said |
@jrtc27 are you happy with this patch in its current form or are there outstanding issues to be addressed?
| llvm/lib/Target/RISCV/RISCVInstrInfo.td | ||
|---|---|---|
| 1015 ↗ | (On Diff #282115) | It's not an indirect branch? The code sequence is identical to PseudoTAIL, just with a non-hard-coded temporary register. Any microarchitecture that does macro-op fusion will recognise this sequence as a PC-relative direct jump. |
I don't know how much it matters, but we know the register is dead here.