Page MenuHomePhabricator

[RISCV] MC relaxation for out-of-range conditional branch.
ClosedPublic

Authored by craig.topper on Aug 30 2021, 5:47 PM.

Details

Summary

If .L1 is not within +-4KiB range,

convert

bge a0, a1, .L1

to

blt a0, a1, 8
j   .L1

In this patch, if the symbol is unresolved at assembly time, do not do
this relaxation.

Fix the bug reported in https://bugs.llvm.org/show_bug.cgi?id=47910

Diff Detail

Event Timeline

HsiangKai created this revision.Aug 30 2021, 5:47 PM
HsiangKai requested review of this revision.Aug 30 2021, 5:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2021, 5:47 PM
HsiangKai edited the summary of this revision. (Show Details)Aug 30 2021, 5:49 PM
HsiangKai edited the summary of this revision. (Show Details)

I know GNU as does this but I don't see why that's a good idea nor why we should; if you use a conditional branch to a label that's too far away then that's your problem and you should have written non-broken code. Having it insert the long branch form is too much magic for me...

asb added a comment.Sep 2 2021, 6:44 AM

I know GNU as does this but I don't see why that's a good idea nor why we should; if you use a conditional branch to a label that's too far away then that's your problem and you should have written non-broken code. Having it insert the long branch form is too much magic for me...

Yes, I'm in two minds about this. I'm usually in favour of trying to match GNU tools behaviour in order to minimise porting issues / provide a more predictable user experience. But fighting against that, I'm really not a fan of these "magic" transformations either.

The fear with adding anything like this is of course adding a very rarely executed path that ends up having bugs in obscure cases. As such, if we were to go in this direction I'd be keen to see more extensive test coverage.

jrtc27 added a comment.Sep 2 2021, 6:58 AM

I know GNU as does this but I don't see why that's a good idea nor why we should; if you use a conditional branch to a label that's too far away then that's your problem and you should have written non-broken code. Having it insert the long branch form is too much magic for me...

Yes, I'm in two minds about this. I'm usually in favour of trying to match GNU tools behaviour in order to minimise porting issues / provide a more predictable user experience. But fighting against that, I'm really not a fan of these "magic" transformations either.

The fear with adding anything like this is of course adding a very rarely executed path that ends up having bugs in obscure cases. As such, if we were to go in this direction I'd be keen to see more extensive test coverage.

FWIW, I *think* the origin of this stems from:

(a) GCC/binutils divide
(b) GCC uses pseudos in its output (even uses li in some cases, though often mixed in with other shifts and adds for decomposing immediates, bit odd)
(c) GCC has limited knowledge of compression

so GCC doesn't actually know how far away things are and thus can't pick the right branch all the time. Fixing it up in the assembler was presumably the easier solution rather than fixing GCC to emit correct assembly. That then has the unfortunate effect of becoming something hand-written assembly relies on (e.g. bbl was, as was FreeBSD in one place very early on before LLVM was mature enough to build it).

asb added a comment.Sep 2 2021, 8:27 AM

FWIW, I *think* the origin of this stems from:

(a) GCC/binutils divide
(b) GCC uses pseudos in its output (even uses li in some cases, though often mixed in with other shifts and adds for decomposing immediates, bit odd)
(c) GCC has limited knowledge of compression

so GCC doesn't actually know how far away things are and thus can't pick the right branch all the time. Fixing it up in the assembler was presumably the easier solution rather than fixing GCC to emit correct assembly. That then has the unfortunate effect of becoming something hand-written assembly relies on (e.g. bbl was, as was FreeBSD in one place very early on before LLVM was mature enough to build it).

That's a good point re the origin of this kind of magic. It probably strengthens the argument in my mind for supporting this - being able to take .s generated by GCC and assemble it with clang/LLVM is a good thing (and we should probably be doing more such testing).

kito-cheng added a comment.EditedSep 2 2021, 9:12 AM

FWIW, I *think* the origin of this stems from:

(a) GCC/binutils divide
(b) GCC uses pseudos in its output (even uses li in some cases, though often mixed in with other shifts and adds for decomposing immediates, bit odd)
(c) GCC has limited knowledge of compression

so GCC doesn't actually know how far away things are and thus can't pick the right branch all the time. Fixing it up in the assembler was presumably the easier solution rather than fixing GCC to emit correct assembly. That then has the unfortunate effect of becoming something hand-written assembly relies on (e.g. bbl was, as was FreeBSD in one place very early on before LLVM was mature enough to build it).

That's a good point re the origin of this kind of magic. It probably strengthens the argument in my mind for supporting this - being able to take .s generated by GCC and assemble it with clang/LLVM is a good thing (and we should probably be doing more such testing).

The description about GCC is not fully correct, GCC also has capability estimate jump range, and convert cond branch to inverted cond branch and a jump during the compilation stage, GCC didn't emit any compressed instruction so the instruction distance estimation is over-conservative.

So when we need this magic?

  1. GCC didn't have integrated assembler like LLVM, so instruction length for inline asm can only use a very roughly way to estimate.
  2. Mostly this is used for hand-written assembly file, RISC-V conditional branch only provide very short range compare to other RISC ISA; +-32K for MIPS, +-32M for ARM32, +-32K(TBZ/TBNZ) ~ +-1M(CBZ/CBNZ) for AArch64, but only +-2K for RISC-V, that made RISC-V is more easy to hit out-of-range condition branch issue, of cause we can ask programmer to convert this by themselves, but out-of-range error is reported until linker time because RISC-V has relaxation, that made many user confuse, (relocation truncate to fit???), this magic can prevent that confusion in most case.
jrtc27 added a comment.Sep 2 2021, 9:18 AM

FWIW, I *think* the origin of this stems from:

(a) GCC/binutils divide
(b) GCC uses pseudos in its output (even uses li in some cases, though often mixed in with other shifts and adds for decomposing immediates, bit odd)
(c) GCC has limited knowledge of compression

so GCC doesn't actually know how far away things are and thus can't pick the right branch all the time. Fixing it up in the assembler was presumably the easier solution rather than fixing GCC to emit correct assembly. That then has the unfortunate effect of becoming something hand-written assembly relies on (e.g. bbl was, as was FreeBSD in one place very early on before LLVM was mature enough to build it).

That's a good point re the origin of this kind of magic. It probably strengthens the argument in my mind for supporting this - being able to take .s generated by GCC and assemble it with clang/LLVM is a good thing (and we should probably be doing more such testing).

The description about GCC is not fully correct, GCC also has capability estimate jump range, and convert cond branch to inverted cond branch and a jump during the compilation stage, GCC didn't emit any compressed instruction so the instruction distance estimation is over-conservative.

So when we need this magic?

  1. GCC didn't have integrated assembler like LLVM, so instruction length for inline asm can only use a very roughly way to estimate.

Is that not a problem for every architecture though?

  1. Mostly this is used for hand-written assembly file, RISC-V conditional branch only provide very short range compare to other RISC ISA; +-32K for MIPS, +-32M for ARM32, +-32K(TBZ/TBNZ) ~ +-1M(CBZ/CBNZ) for AArch64, but only +-2K for RISC-V, that made RISC-V is more easy to hit out-of-range condition branch issue, of cause we can ask programmer to convert this by themselves, but out-of-range error is reported until linker time because RISC-V has relaxation, that made many user confuse, (relocation truncate to fit???), this magic can prevent that confusion in most case.

There's nothing stopping an assembler from detecting that a label will/won't/might be out of range for a given branch by doing best-case and worst-case analysis on the instruction sequence and emitting an error in the case where it's definitely going to be out of bounds however much you relax, and a warning when it's unsure because it depends on how much relaxation there is (which is likely to be relatively rare, normally these cases are _way_ out of bounds for user-provided assembly).

So when we need this magic?

  1. GCC didn't have integrated assembler like LLVM, so instruction length for inline asm can only use a very roughly way to estimate.

Is that not a problem for every architecture though?

Yeah, but RISC-V is more frequently hit due to shorter range for conditional branch, I believe integrated assembler is right way to resolve this, but sadly it's not existing in GCC :(

  1. Mostly this is used for hand-written assembly file, RISC-V conditional branch only provide very short range compare to other RISC ISA; +-32K for MIPS, +-32M for ARM32, +-32K(TBZ/TBNZ) ~ +-1M(CBZ/CBNZ) for AArch64, but only +-2K for RISC-V, that made RISC-V is more easy to hit out-of-range condition branch issue, of cause we can ask programmer to convert this by themselves, but out-of-range error is reported until linker time because RISC-V has relaxation, that made many user confuse, (relocation truncate to fit???), this magic can prevent that confusion in most case.

There's nothing stopping an assembler from detecting that a label will/won't/might be out of range for a given branch by doing best-case and worst-case analysis on the instruction sequence and emitting an error in the case where it's definitely going to be out of bounds however much you relax, and a warning when it's unsure because it depends on how much relaxation there is (which is likely to be relatively rare, normally these cases are _way_ out of bounds for user-provided assembly).

Technically that's feasible solution, but I think that's would be more like philosophical issue here :p

So when we need this magic?

  1. GCC didn't have integrated assembler like LLVM, so instruction length for inline asm can only use a very roughly way to estimate.

Is that not a problem for every architecture though?

Yeah, but RISC-V is more frequently hit due to shorter range for conditional branch, I believe integrated assembler is right way to resolve this, but sadly it's not existing in GCC :(

I don't particularly like adding magic to RISC-V just because it hits an issue shared by many architectures more often than them. If I can break every architecture other than RISC-V with a sufficiently-large blob of inline assembly then it seems to me like GCC needs fixing, not that RISC-V should have a special hacky workaround just for it that gets exposed to and relied upon by users?

  1. Mostly this is used for hand-written assembly file, RISC-V conditional branch only provide very short range compare to other RISC ISA; +-32K for MIPS, +-32M for ARM32, +-32K(TBZ/TBNZ) ~ +-1M(CBZ/CBNZ) for AArch64, but only +-2K for RISC-V, that made RISC-V is more easy to hit out-of-range condition branch issue, of cause we can ask programmer to convert this by themselves, but out-of-range error is reported until linker time because RISC-V has relaxation, that made many user confuse, (relocation truncate to fit???), this magic can prevent that confusion in most case.

There's nothing stopping an assembler from detecting that a label will/won't/might be out of range for a given branch by doing best-case and worst-case analysis on the instruction sequence and emitting an error in the case where it's definitely going to be out of bounds however much you relax, and a warning when it's unsure because it depends on how much relaxation there is (which is likely to be relatively rare, normally these cases are _way_ out of bounds for user-provided assembly).

Technically that's feasible solution, but I think that's would be more like philosophical issue here :p

MaskRay added a comment.EditedSep 2 2021, 10:28 AM

Some objection from me.

x86 does have such a relaxation (breaking WYSIWYG), but this is not a thing that any other RISC arch I know does.
WYSIWYG matters a bit because aligned branched for loops may have performance advantage. Unpredictable assembler relaxation can break it.
(On x86 when Intel jump condition code erratum was discussed) it is said certain instructions have "side effect after exactly one or two instruction", arbitrarily replacing one instruction has some risk. )
thumb1 has a short range (+-256) as well, but it doesn't use assembler relaxation:

# thumb: Error: branch out of range
.thumb
beq .Lfoo
.rept 129
nop
.endr
.Lfoo:

RISCV already uses lib/CodeGen/BranchRelaxation.cpp on the compiler side, so I am not sure why we need this assembler side solution.

https://gcc.gnu.org/onlinedocs/gcc/Size-of-an-asm.html#Size-of-an-asm says
"It does this by counting the number of instructions in the pattern of the asm "
Multiple RISC ports use this. If GCC RISCV doesn't do this yet, it should be fixed.

For completeness, such long branches are typically not performance bottleneck.
Even if linker relaxation can shorten the distance a bit and allow more short-form branches,
it may not worth the complexity.

Will the BranchRelaxation pass account for .insn directives in inline asm correctly? Or will we need to do additional work for that associated with D108602

Will the BranchRelaxation pass account for .insn directives in inline asm correctly? Or will we need to do additional work for that associated with D108602

I haven't looked at it closely yet. BranchRelaxation.cpp uses TargetInstrInfo::getInstSizeInBytes to compute the size.

craig.topper edited the summary of this revision. (Show Details)Sep 2 2021, 10:42 AM

https://gcc.gnu.org/onlinedocs/gcc/Size-of-an-asm.html#Size-of-an-asm says
"It does this by counting the number of instructions in the pattern of the asm "
Multiple RISC ports use this. If GCC RISCV doesn't do this yet, it should be fixed.

That's not target dependent feature, and it's roughly way to estimate, unless GCC has something like MC layer, otherwise it's never accurate.

For completeness, such long branches are typically not performance bottleneck.
Even if linker relaxation can shorten the distance a bit and allow more short-form branches,
it may not worth the complexity.

RISC-V don't have special relocation to handle this relaxation, and binutils didn't handle that, although that could be recognized by R_RISCV_BRANCH + R_RISCV_JAL pair, but as you said it's might be complicated and not worth.

It is not a new feature for RISC-V. We already have c.beqz to beq, c.bnez to bne, c.j to jal, etc. This patch just extends the relaxation patterns to conditional branches. If MC relaxation is not a good idea, should we remove all the patterns in the relaxInstruction target hook?

jrtc27 added a comment.Sep 6 2021, 6:45 PM

It is not a new feature for RISC-V. We already have c.beqz to beq, c.bnez to bne, c.j to jal, etc. This patch just extends the relaxation patterns to conditional branches. If MC relaxation is not a good idea, should we remove all the patterns in the relaxInstruction target hook?

My understanding is that's more a quirk of our implementation and is there to un-compress branches that get erroneously compressed (since a bare symbol is a valid operand for compressed branches), but yes, that does currently cause things to behave a bit strangely and ideally wouldn't be there.

binutils also does this for at least Xtensa http://sourceware.org/binutils/docs/as/Xtensa-Branch-Relaxation.html

There was this pull request to document the GNU behavior in the riscv-asm-manual https://github.com/riscv-non-isa/riscv-asm-manual/pull/58/files

The inline assembly counting in LLVM is known to be incomplete see https://bugs.llvm.org/show_bug.cgi?id=42539

I think maybe .insn will work with the current implementation of getInlineAsmLength because it mostly just counts the number of lines and multiplies by MaxInstLength. With a special case for .space directive.

binutils also does this for at least Xtensa http://sourceware.org/binutils/docs/as/Xtensa-Branch-Relaxation.html

There was this pull request to document the GNU behavior in the riscv-asm-manual https://github.com/riscv-non-isa/riscv-asm-manual/pull/58/files

The inline assembly counting in LLVM is known to be incomplete see https://bugs.llvm.org/show_bug.cgi?id=42539

I think maybe .insn will work with the current implementation of getInlineAsmLength because it mostly just counts the number of lines and multiplies by MaxInstLength. With a special case for .space directive.

If we deal with special directives such as .rept in getInlineAsmLength, we should be able to get the correct branch instruction according to the estimated offset. I agree that the compiler should be responsible to generate the correct code, not depends on MC to fix it. However, hand-written assembly code is another story. How about to turn MC relaxation off by default and let users turn on it if users want the compiler to help them to take care of these transformation?

I found some pseudo instruction are expanded to multiple MI instructions in RISCVExpandPseudoInsts.cpp. And the BranchRelaxation pass will run before the expandPseudo pass. Is it possible to cause the fixup value out-of-range on some branch instructions? If this will happen, maybe add MC layer branch relaxation can deal with it.

I found some pseudo instruction are expanded to multiple MI instructions in RISCVExpandPseudoInsts.cpp. And the BranchRelaxation pass will run before the expandPseudo pass. Is it possible to cause the fixup value out-of-range on some branch instructions? If this will happen, maybe add MC layer branch relaxation can deal with it.

Not unless RISCVInstrInfo::getInstSizeInBytes has missing or incorrect cases.

I found some pseudo instruction are expanded to multiple MI instructions in RISCVExpandPseudoInsts.cpp. And the BranchRelaxation pass will run before the expandPseudo pass. Is it possible to cause the fixup value out-of-range on some branch instructions? If this will happen, maybe add MC layer branch relaxation can deal with it.

Not unless RISCVInstrInfo::getInstSizeInBytes has missing or incorrect cases.

Got it. Thanks!

Zeavee added a subscriber: Zeavee.Jul 13 2022, 7:33 AM

Hello, we are currently trying to implement an LLVM backend on RISC-V for the GraalVM Native Image project and we run into some issues that are solved by this patch. In summary, we produce LLVM bitcode from Java code and feed it to llc. The problem is that we cannot really control the size of the functions and it can cause branches to out of range locations. However, this issue seems stale and controversial. Is there any news to share?

Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 7:33 AM

Hello, we are currently trying to implement an LLVM backend on RISC-V for the GraalVM Native Image project and we run into some issues that are solved by this patch. In summary, we produce LLVM bitcode from Java code and feed it to llc. The problem is that we cannot really control the size of the functions and it can cause branches to out of range locations. However, this issue seems stale and controversial. Is there any news to share?

llc has branchrelaxation, doesn't it solve your problem?

This comment was removed by Zeavee.
craig.topper commandeered this revision.Fri, Jan 27, 1:52 PM
craig.topper edited reviewers, added: HsiangKai; removed: craig.topper.

Rebase code. Still waiting on build to check tests.

Fix build error

@reames has discovered that CodeGen's BranchRelaxation doesn't understand how RISC-V emit alignment directives when relaxation is enabled. Align directives always emit (align - mininstsize) bytes of NOPs. The linker is responsible for cleaning up the extra NOPs to meet the requested alignment. BranchRelaxation doesn't know this and undercounts the size of the alignment NOPs. This shows up when -falign-loops is used for example.

Adding this relaxation to the assembler is the quickest fix and matches the GNU assembler anyway. Though it still wouldn't handle the case that requires a branch to become indirect.

lit tests are clean with this patch

As @craig.topper mentioned above, I stumbled into this issue last week when debugging an LTO failure. The exact issue I hit was because BranchRelaxation doesn't account for the padding required in RISCV's alignment directive. (Which is different from every other target.) We could, of course, patch the particular issue in BranchRelaxation, but I want to make an argument for reversing course on this patch as well. If we'd landed this patch, my hard to debug LTO correctness issue would have been a minor code quality issue at worst, and I'd not have lost a week of my life. :)

First, we are currently incompatible with the GNU toolchain. LLVM's assembler will not successfully assemble programs which 'as' will. This shows up both in hand written assembly, but also, in theory at the moment, in output from gcc.

target:
	bne a0, a1, target
.rep 1024
	nop
.endr
	bne a0, a1, target

Personally, I consider this a very strong reason to accept this patch (or some variant thereof). To the point where I almost hesitate to bring up other lines of arguments because I'm worried they'll distract from the core point of cross toolchain compatibility. We *need* cross toolchain compatibility; any exceptions to this need to be *very* strongly justified, and I don't consider any of the arguments made on this thread to date to come anywhere close to that bar.

Second, I don't believe the correctness invariant relied on from BranchRelaxation is a good idea. At the moment, we *must* get all instruction sizes used in branch relaxation to be upper bounds on the actual bytes generated. We've now had multiple examples where this was not upheld in edge cases, and I don't see anything in the code which particularly enforces said invariant. I've posted a couple patches to add a few relevant asserts here and there, but this is a cross cutting invariant with broad implications. If we can remove it it, that removes a major correctness risk.

There was an argument made on thread previously that this patch adds rarely executed behavior which is likely to bit rot. I think this argument gets things completely backwards. The fundamental case requiring relaxation has to be implemented somewhere, and the assembler implementation is actually the easier to test of the alternatives. The assembler change has dramatically less "blast radius" than the compiler BranchRelaxation approach.

Now, I do need to acknowledge that there's related branch relaxation case which the assembler *can't* easily handle. Specifically, when we cross out of +/- 2MB range, we need a scratch register to materialize the long branch. To my knowledge, GCC doesn't handle this case at all.

Third, I think we actually have potentially significant missed optimizations here. BranchRelaxation is inherently conservative. As one example, @craig.topper tells me that every branch is modeled as 4 bytes, even if the branch target would allow the branch to be compressible. As such, for branch dense code (e.g. loop nests) we can end up relaxation branches which don't need relaxed. I haven't (yet) seen a case compelling enough on it's own to address, but in theory, such cases are certainly possible. Since we have a correctness need to change schemes here anyways, we might as well fix the optimization problem at the same time.

reames added inline comments.Mon, Jan 30, 8:54 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
199

Is this right? BEQ is not the same as BEQZ? Isn't there a missing zero check here?

Ah, no, that's checked in the caller. Could you adjust naming or comments to make that clear please? In fact, so much of the caller differs for this case, maybe it should just be an if/else block there?

229

Doing this when we're not going to use the compressed instruction seems like an odd canonicalization from the assembler. Not objecting, just wondering why.

276

Can you precommit a change which pulls this out and converts these branches to a switch? Doing so reduces the delta.

llvm/test/MC/RISCV/long-conditional-jump.s
7

Use check-prefixes to common all but the couple which actually differ?

craig.topper added inline comments.Mon, Jan 30, 9:33 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
229

Probably because it made it easier to write the expression for UseComperssBr as a single condition. I'll rework it.

Rebase after adding switch to encodeInstruction. I'll work on other comments next.

asb added a comment.Mon, Jan 30, 11:01 AM

As @craig.topper mentioned above, I stumbled into this issue last week when debugging an LTO failure. The exact issue I hit was because BranchRelaxation doesn't account for the padding required in RISCV's alignment directive. (Which is different from every other target.) We could, of course, patch the particular issue in BranchRelaxation, but I want to make an argument for reversing course on this patch as well. If we'd landed this patch, my hard to debug LTO correctness issue would have been a minor code quality issue at worst, and I'd not have lost a week of my life. :)

I'm very sorry for that, I know from personal experience how galling it is to spend a long time debugging something to later find a patch that fixed it had already been proposed. I don't personally have a reliable system for tracking patches that don't get re-pinged by their authors, but there was a bug report for this issue here, so definitely a failure there...

There was an argument made on thread previously that this patch adds rarely executed behavior which is likely to bit rot. I think this argument gets things completely backwards. The fundamental case requiring relaxation has to be implemented somewhere, and the assembler implementation is actually the easier to test of the alternatives. The assembler change has dramatically less "blast radius" than the compiler BranchRelaxation approach.

I wanted to clarify this point - my concern was adding a rarely executed and insufficiently tested code path (which has in the past also been the source of hard to debug issues).

Address review comments

craig.topper added inline comments.Mon, Jan 30, 11:35 AM
llvm/test/MC/RISCV/long-conditional-jump.s
7

There doesn't appear to be much common. The hex constants are different in every case but the first.

llvm/test/MC/RISCV/long-conditional-jump.s
7

You are correct here. Ignore me.

Code wise, LGTM. Please wait on landing until we settle the high level approach questions.

asb added a comment.Tue, Jan 31, 11:21 AM

Code changes LGTM as well. I can't think of other cases worth testing either (but of course, more eyes always welcome there).

Vote +1 from my personal side.
We met some issues when porting Android/Rust to RISCV, and finally found it can be fixed by this patch.

reames added a comment.EditedThu, Feb 2, 11:17 AM

We discussed this today at the RISCV sync up meeting. Unfortunately, @jrtc27 wasn't in attendance.

Out of the attendees, @asb, @craig.topper, and I spoke in support of landing this. @luismarques remembered being hesitant, and was going to revisit.

I'm going to reach out to @jrtc27 to discuss, and see if her previous opposition still stands given new information. (edit: Reached out via email on 2/2/23)

luke957 removed a subscriber: luke957.Thu, Feb 2, 9:55 PM

To summarise my position after chatting things through with Philip, I'm in favour of landing this as-is^. For compiler-generated code, I would personally rather it not be something relied upon, but I understand the reasons it does from a practical perspective and that others have differing philosophical positions. My bigger concern was with hand-written assembly, especially with .option norelax (and possibly .option norvc) where there is the expectation that what you write is exactly what you get at the byte level, not just semantically (c.f. Linux's alternatives and static branches, which make use of that). For compatibility with GNU as on real-world code we do unfortunately need to support this, and so having it on by default is the way we have to go. I would, however, like to see both assemblers provide a standard way to turn this off in future, and perhaps an eventual path for compilers to explicitly opt-in to using this such that the default could be changed many years down the road (and thus not "surprise" developers), but that's all future work that isn't blocking for any of this.

^ NB: This is not a code review though, I haven't studied the changes themselves, hence making this just a comment and not an "official" approval

asb accepted this revision.Fri, Feb 3, 10:21 AM

Thanks @jrtc27 - marking this explicitly as LGTM now the remaining concerns have I think all been addressed.

This revision is now accepted and ready to land.Fri, Feb 3, 10:21 AM
MaskRay added inline comments.Fri, Feb 3, 10:43 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
153

isInt<13>(Offset).

(Offset > 4094 || Offset < -4096); is not different from (Offset > 4095 || Offset < -4096);

llvm/test/MC/RISCV/long-conditional-jump.s
2

< %s => %s

5

-triple=riscv64

Consistently use = or separator.

craig.topper added inline comments.Fri, Feb 3, 11:09 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
153

Seems like this comment would apply to line 168 and 172 as well?

Address @MaskRay's comments

This revision was landed with ongoing or failed builds.Fri, Feb 3, 12:34 PM
This revision was automatically updated to reflect the committed changes.