In D65205#1600772, @lenary wrote:

Is there a reason not to include them in the instruction operand string? It would mean you don't need the custom printer, no?

This is actually really complex. If I include the parens in the tablegen instruction definition, then we definitely cannot support 0(a0) because the parser requires a ( where the 0 is. If i don't include them in the instruction definition, then when parseMemBaseOp runs, it adds ( tokens into the Operands, which means later the register operand is not in the expected place in Operands when operand matching happens.

In D65205#1599193, @lenary wrote:

In D65205#1599133, @jrtc27 wrote:

Yeah this looks like a nice way to do it, better than making an InstAlias for each one. GNU as will accept any expression that evaluates to the constant 0 in place of the 0 (i.e. it's just an imm0, same logic as all the other [su]immX fields, other than discarding it). Could we not therefore reuse part of the generic parser, just with a tweak to drop the immediate if parsed and 0? Something like:

... snip ...

I am worried about this approach, mostly because of the lines like 1268 in parseMemOpBaseReg, where they're pushing on (/) tokens, which aren't in the instruction operand strings (any more). It does seem like these might complicate the pushing/popping more than necessary. I am also not sure we want to support bare register operands.

Yeah this looks like a nice way to do it, better than making an InstAlias for each one. GNU as will accept any expression that evaluates to the constant 0 in place of the 0 (i.e. it's just an imm0, same logic as all the other [su]immX fields, other than discarding it). Could we not therefore reuse part of the generic parser, just with a tweak to drop the immediate if parsed and 0? Something like:

@asb I prefer D60657 over this as it seems to cover everything, and this is a rather big hammer to be hitting the problem with (that GNU as manages to avoid needing). Have you had a look at that one?

Rebased

Seems like a good idea

Looks sensible to me

I don't like the mismatch between LLD and BFD, but this is a definite improvement over the current state. I'm happy for this to be merged, with the intent that discussions about CALL/CALL_PLT continue in the RISC-V ABI world to get a consensus, specify it and then fix any implementations that deviate from it, whether that be BFD, LLD or both.

Added nounwind to tests. Added full stops to comments.

One minor pedantic stylistic comment but otherwise this seems fine to me.

In D57792#1549678, @asb wrote:

In D57792#1549676, @jrtc27 wrote:

In D57792#1549669, @asb wrote:

Is there documentation on the operand modifiers supported by RISC-V binutils anywhere? I'm struggling to find anything...

This is GCC not binutils, and I can't find them documented anywhere other than the source itself: https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/config/riscv/riscv.c?view=markup&revision=271293#l3181

Sorry, I meant GCC.

Thanks, this patch looks good to me. Tiny nit: LLVM coding guidelines prefer full sentences for comments, so the comments added to RISCVAsmPrinter.cpp should end in a full stop.

In D57332#1549661, @asb wrote:

Is this being used in the wild and does it work sensibly with binutils? I'm seeing that binutils seems to be associating the addend with both the emitted R_RISCV_PCREL_HI20 relocation and R_RISCV_RELAX relocation. Maybe this is just a binutils bug and the addend is ignored by the linker but this does seem surprising...

$ cat t.s 
lla a5, a_symbol + (0xFF << 3)
$ ./riscv32-unknown-elf-as t.s
$ ./riscv32-unknown-elf-readelf -r a.out 

Relocation section '.rela.text' at offset 0xec contains 4 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  00000617 R_RISCV_PCREL_HI2 00000000   a_symbol + 7f8
00000000  00000033 R_RISCV_RELAX                7f8
00000004  00000418 R_RISCV_PCREL_LO1 00000000   .L0  + 0
00000004  00000033 R_RISCV_RELAX                0

In D57792#1549669, @asb wrote:

Is there documentation on the operand modifiers supported by RISC-V binutils anywhere? I'm struggling to find anything...

In D63076#1545858, @ruiu wrote:

Does that make a visible difference?

Still disagree about the behaviour of R_RISCV_CALL. You can't just choose the ABI that you want. Please at least put it back to R_PC. @ruiu do you have an opinion on this?

In D63132#1541242, @MaskRay wrote:

@jrtc27 Do this patch and D63076 look good now?

In D63132#1538147, @MaskRay wrote:

GNU ld treats it as undefined:

Yes. That was why I created the patch to fix the musl -fpie -pie link error: ld.lld: error: relocation R_RISCV_PCREL_HI20 cannot refer to absolute symbol: __global_pointer$. This error is lld specific: in -pie/-shared mode, a PC relative relocation to an absolute symbol is not allowed.

In https://groups.google.com/forum/#!searchin/generic-abi/SHN_ABS|sort:date/generic-abi/MC4b4sqnKC0/oQgZxxwMmJAJ, Cary's interpretation of gABI is:

On the contrary, the only interpretation that makes sense to me is that it will not change because of relocation at link time or at load time.

If lld resolves the PC relative relocation statically, the value will be incorrect when the pie or shared object is loaded at runtime because the SHN_ABS symbol's value does not change. The best lld can do is to issue an error.

An alternative is to create a dummy .sdata in musl's Scrt1.o. I don't know how reasonable it is to have lla gp,__global_pointer$ in other code that might not define .sdata.

GNU ld treats it as undefined:

Sorted OS case statements

Hm, actually, looking more closely, there seems to be inconsistency. Some backends use FooELFStreamer.cpp for an MCELFStreamer, and others for a ELF-specific target streamer. Some, like AArch64, even put both in the same file, so if you don't like this patch, I suppose the recommendation would be to do that?

In D61866#1532288, @asb wrote:

I think this is less confusing, but there's more to be gained by having a similar naming scheme to the other backends which right now all use FooELFStreamer.{cpp,h}.

I you feel strongly about this change, I'd consider making it for all in-tree backends.

Ping?

In D60657#1475609, @lewis-revill wrote:

Thanks for this. I realize D58843 is a very heavy-handed approach so I'd be happy if we could avoid it cleanly with target-specific changes, however I'm not certain we would be able to cover all the combinations we would need to with patches like this?

For example would we be able to cover pairs that occur out of order?

Why do we need this flag; can we not already determine this (either in generic code or the target backends)? Anything where all symbols referenced are not in SHF_EXECINSTR sections can be evaluated even with relaxations, which covers all these cases:

Ouch; LGTM.

Also handle .bss where the zeroes are written to the file.

The problem is that Length is the length of the CIE, i.e. the data structure itself; this means it isn't subject to relaxations and so the difference expression can be folded. I don't see much merit in teaching all the tools out there to check relocations for Length when we can instead just emit the "right" thing. I think the fix is to make sure we only return true from requiresDiffExpressionRelocations if a target is in a code section? MCSection::UseCodeAlign will tell you this (or you can use MCSectionELF::getFlags() & ELF::SHF_EXECINSTR to be less general), though perhaps it should be renamed to isCodeSection to match isVirtualSection.

Updated affected tests.

One minor comment, but otherwise I think this is ready to land now?

@PkmX Ping, any chance you could address the remaining few points, please?

@PkmX Ping, any chance you could address the remaining few points, please?

This probably needs some invalid tests (ie the out of range immediates mentioned in the TODO). Also, please run utils/update_llc_test_checks.py to get the RV64I test lines.

A separate point is whether it makes sense to be emitting this warning in the first place for GE_Missing_type. I'd argue that, if we don't know the type of the builtin, we should never emit the warning, since otherwise we always emit the warning even for legitimate instances, though that's not really important now we're not hitting that case any more.

D50496 in the stack has now landed, so other than the TODO comments having been dropped this is now ready. Alex, are you happy to do that rebasing when you commit this, or would you like me to?

Also worth noting that the frontend needs to know the max width otherwise warn_atomic_op_misaligned will be triggered with the message large atomic operation may incur significant performance penalty (name is misleading since the %select{large|misaligned}0 means it's triggered for both large and misaligned atomic ops, and it's also in the atomic-alignment group). This is exactly what I did in my tree (other than the tests) and it seemed to work correctly for RV64A compiling FreeBSD.

Perhaps we should keep getKindForSize as being in bytes, and add a new getKindForSizeInBits? This would reduce the diff, but more importantly ensures that downstream forks don't silently break until runtime due to the changed semantics of getKindForSize despite having the same type as before.

In D54093#1397875, @asb wrote:

Did you check the behaviour of 'J' vs GCC, because for e.g. asm volatile ("sub a0, a0, %0" :: "J"(0)); I seem to see it using the integer 0 rather than the zero register?

I think it would be worth adding RV64I check lines to inline-asm.ll too.

I think we should introduce a new Constraint_A enum member for this case. Whilst "A" currently behaves the same for the RISC-V backend as "m", that won't necessarily be the case forever. "A" is required to always be a single GPR (and so can be used for the atomic instructions, or ones with reduced immediate encoding space), but "m" (at least according to GCC) is "any kind of address that the machine supports in general", which I assume for RISC-V means reg+simm12.

Please update docs/ClangCommandLineReference.rst. Also, in my limited testing, GCC just seems to pass the -msmall-data-limit=... flag straight through to the linker through COLLECT_GCC_OPTIONS. Finally, please add tests for -msmall-data-limit=... instead of or as well as -G, since GCC only recognises the former on RISC-V.

Fixed broken test

Support all memory instructions; add test coverage

In D57240#1385323, @lewis-revill wrote:

In D57240#1385261, @jrtc27 wrote:

Could we not have a situation where the pcrel_hi20 is seen *after* the lo12, in which case the lo12 would be forced but the hi20 could still be evaluated? E.g. I think the following would be problematic:

foo:
    j 1f
0:  addi a0, a0, %pcrel_lo(1f)
    ret
1: auipc a0, %pcrel_hi(bar)
    j 0b

.align 2
bar:
    # ...

Obviously extremely pathological code, but nonetheless valid.

Good call. It's a pathological case but maybe there are other cases as well. It's not a nice situation. Somehow we have to ensure there can never be a pcrel_lo12 relocation without a corresponding pcrel_hi20 relocation. It might help to know whether the pcrel_hi20 has been seen at all, regardless of whether it was resolved.

So I guess the intended behaviour might be:

pcrel_hi20 seen and not resolved (IE relocation emitted) -> force relocation
pcrel_hi20 seen and resolved -> don't force relocation
pcrel_hi20 not seen -> don't force relocation? If evaluatePcRelLo thinks it can be evaluated does it matter if the pcrel_hi20 can or not?

I'll have another think and take a look at your testcase.

Please rebase now I've removed the stray whitespace in the definition of PseudoLA.

Hi Lewis, please rebase this after rL351823 (the Call node is now a riscv_call node in PseudoCll's pattern.

Hi Lewis, could you please rebase this after the RV64A patch landed and added PseudoMaskedCmpXchg64 to RISCVExpandPseudoInsts (conflicts with adding PseudoLLA)?

(Added dependency due to both adding to RISCVBaseInfo.h)

Rebased and added TODO comment.

Rebased, fixed whitespace, removed TODO, added FIXME, added invalid tests.

In D55279#1367361, @asb wrote:

Hi James, can you please rebase and confirm that you are aware of the new license put in place last Friday and intend to contribute under it. See http://lists.llvm.org/pipermail/llvm-dev/2019-January/129344.html https://llvm.org/docs/DeveloperPolicy.html#new-llvm-project-license-framework

Rebased (sorrt for the delay).

Could we not have a situation where the pcrel_hi20 is seen *after* the lo12, in which case the lo12 would be forced but the hi20 could still be evaluated? E.g. I think the following would be problematic:

You should save the original AsmToken rather than reconstructing it, so as to retain the SMLoc, and Buf no longer needs to be at function scope. Also, I had some more exhaustive tests in mine; I think the important one is addi a1, a0, (1) to ensure that we can parse parenthesised immediate expressions when *not* part of a memory reference.

There's also AsmToken::Exclaim for logical not that we could allow (although that's not one I've ever seen in real-world assembly). Please add tests for the CSR change?

Ah, I already did this in my fork, covering all the compressed and floating-point instructions too, with a complete set of tests. Probably easiest if I just post that rather than you working to produce what would turn out to be nearly-identical code?

In D50496#1358355, @apazos wrote:

Just noted that this is failing:

"sw zero, (a0)"   
<stdin>:1:15: error: operand must be a symbol with %lo/%pcrel_lo modifier or an integer in the range [-2048, 2047]
sw      zero, (a0)
              ^

We should also accept it as sw zero, 0(a0).
I verified GNU accepts it.