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.

Thanks, looks good to me now.

In D54029#1329871, @PkmX wrote:

@jrtc27 Actually I agree it should produce an error regardless of whether relaxation is enabled, since it is most likely an user error to write a %pcrel_lo that doesn't point to an auipc, and the earlier we can catch it the better.

In D54029#1329560, @asb wrote:

This looks good to me, but there's one addition test I'd like to see. You explain that the intended behaviour is that there will be no compile-time error if linker relaxation is enabled. It would be good to have this reflected in a test. Maybe adding extra RUN lines to pcrel-lo12-invalid.s and adding a comment that explains that is expected with linker relaxation enabled.

In D55296#1319323, @jrtc27 wrote:

In D55296#1319274, @efriedma wrote:

Okay, then LGTM

I don't have commit rights, so could somebody please commit on my behalf?

Oh, probably also worth mentioning the small <-> medlow and medium <-> medany GCC code model mapping in the commit message, and probably a comment too in the source code of getAddr.

Seems fine from my point of view but we'll wait to hear from the others. I'd suggest removing the "WIP" from the subject to reflect the true state.

Almost there now from my point of view, but we'll see what the others say (especially Alex).

Rebased

Slight annoying bit of churn turning getPCRelHiExpr into getPCRelHiFixup (when I tweaked D54029 I simply didn't make shouldForceRelocation give an error for a null getPCRelHiExpr return value, and therefore it could remain unchanged, but with the increased error checking we now need to get the entire MCFixup back).

Rebased on top of latest changes to D54029

In D55279#1326559, @lewis-revill wrote:

In D55279#1326556, @lewis-revill wrote:

Do you need to add a check for RISCV::fixup_riscv_pcrel_hi20 in getPcRelHiExpr? Currently this is returning null and causing a segfault when running your relocations.s tests.

RISCV::fixup_riscv_pcrel_hi20 ->RISCV::fixup_riscv_got_hi20

In D55341#1321157, @luismarques wrote:

GCC/binutils allows add a0, a0, tp, 0. I've submitted a bug against binutils [1] which attempts to clarify if that should actually be supported or not. Depending on the feedback, this patch's requirement that %tprel_add needs to always be present might need to be revised.
[1] https://sourceware.org/bugzilla/show_bug.cgi?id=23956

Please also add some tests to relocations.s like I have done for %got_pcrel_hi.

Rebased on top of D54029, added test case to ensure R_RISCV_GOT_HI20 is always preserved for the linker and updated error message for InvalidUImm20AUIPC.

In D55335#1320688, @lewis-revill wrote:

Ideally no. I guess it would be good to error when parsing rather than trying to emit a R_RISCV_CALL_PLT relocation for a non-call... I'm leaning towards checking Operands[0] at the moment but it depends what others think.

In D54143#1322247, @lewis-revill wrote:

In D54143#1322155, @jrtc27 wrote:

In D54143#1321605, @lewis-revill wrote:

So my problem is that (if we expand PseudoLLA in codegen in the same way as we do in RISCVAsmParser) then we lose some flexibility for later addressing uses. Currently I can use the wrapper to do (WrapperPCRel %pcrel_hi(sym)), (WrapperPCRel %got_pcrel_hi(sym)), (WrapperPCRel %tls_ie_pcrel_hi(sym)) and (WrapperPCRel %tls_gd_pcrel_hi(sym)), whereas with PseudoLLA I am limited to just %pcrel_hi addressing. The addition of PseudoLA would help greatly, but only for PIC, not for TLS, meaning another wrapper would be required anyway. Maybe this is an acceptable approach to use PseudoLLA and PseudoLA where it fits then add a wrapper later?

For TLS you'd then use the yet-to-be-implemented PseudoLA_TLS_GD and PseudoLA_TLS_IE (or whatever they end up being called) representing the la.tls.gd and la.tls.ie macros/pseudo-instructions. Anything you could want to use WrapperPCRel will need a corresponding PseudoFOO to exist for the assembly parser, so I still fail to see why we would need this extra wrapper. I really like having the exact correspondence between what CodeGen produces and what the equivalent hand-written assembly would be parsed to.

Thanks, this clears things up for me. I understand why that would be the better approach now. I did see that those pseudo instructions are what GCC produces but since they weren't part of the RISCV specifications I didn't think about using them in LLVM. I'll try to make these changes and add patches where necessary.

In D54143#1321605, @lewis-revill wrote:

So my problem is that (if we expand PseudoLLA in codegen in the same way as we do in RISCVAsmParser) then we lose some flexibility for later addressing uses. Currently I can use the wrapper to do (WrapperPCRel %pcrel_hi(sym)), (WrapperPCRel %got_pcrel_hi(sym)), (WrapperPCRel %tls_ie_pcrel_hi(sym)) and (WrapperPCRel %tls_gd_pcrel_hi(sym)), whereas with PseudoLLA I am limited to just %pcrel_hi addressing. The addition of PseudoLA would help greatly, but only for PIC, not for TLS, meaning another wrapper would be required anyway. Maybe this is an acceptable approach to use PseudoLLA and PseudoLA where it fits then add a wrapper later?

I realise now this is also dependent on D54029, otherwise the corresponding %pcrel_lo gets evaluated to the offset of the auipc. I'll rebase on top of that.

In D55279#1320388, @lewis-revill wrote:

Could you correct the error message for InvalidUImm20AUIPC in RISCVAsmParser to include this new operator & change rv32i-invalid.s appropriately? When I did this I also added a test line in rv32i-valid.s but it's mostly testing the same thing as the test in relocations.s. Maybe you want to add that as well for completeness though.

In D55303#1320326, @lewis-revill wrote:

It's probably worth noting that the %pcrel_lo relocations to the label appear to be evaluated to a constant when -mattr=+relax is not provided... I remember someone said that this was going to be made default? If so I think it would be good to wait for that before this is commited.

In D54143#1321182, @rogfer01 wrote:

My only problem with that approach is that it seems wrong to expand PseudoLLA the same way I am expanding PseudoAddrPCRel, IE allowing the AUIPC operand to be decided by codegen.

I'm not sure to follow here.

I think @jrtc27 means that, instead of adding a new PseudoAddrPCRel and select it from a WrapperPCRel, we could select PseudoLLA and then expand it in RISCVExpandPseudoInsts.

That said, I presume at some point we will want to add codegen for GOT-addressing. We have a few options here but if we reuse the WrapperPCRel and we use a different target flag to record that this is a GOT-relocation, then the expansion of PseudoLLA in the codegen flow and the asm parser flow will be different (the latter always doing %pcrel_hi while the former might be able to do both %pcrel_hi / %got_pcrel_hi). This does not seem ideal to me. Adding another target-specific DAG (e.g. WrapperGOTRel) node is workable but feels unnecessary.

Yes, this is a stupid situation to be in, but 32-bit PowerPC on SUSE really does use /usr/lib/gcc/powerpc64-suse-linux:

The general strategy of adding a pseudo-instruction seems fine; I checked what the Sparc backend does and it defines a tlsadd node with its extra %tie_add(sym) operand.

I'd rather see all the local-exec support moved into D55306; I don't think it makes much sense to add things here that don't yet work. Alternatively, to avoid lines that are then going to be modified again in D55306, you could merge the two? There aren't many extra changes in D55306.

Half of this (the MC side of the GOT modifier) is already proposed in D55279. Also note that the assembly modifier is not %got_hi but %got_pcrel_hi after a discussion with upstream (check the assembler manual) to clearly convey that it's a PC-relative relocation to the GOT entry rather than generating a GOT index as would be the case on some older architectures. I did however, like you, use GOT_HI in the various LLVM-internal enumeration entries to match the name of the relocation.

In D54143#1319527, @lewis-revill wrote:

Rebased on top of master, and added entry to getInstSizeInBits for PseudoAddrPCRel.

In D54143#1319494, @lewis-revill wrote:

I've tried re-using PseudoLLA, but I just cannot get around the problem of expanding it in the MC layer. If I try expanding it in RISCVMCCodeEmitter there is no way to get/create an appropriate expression to use for the %pcrel_lo relocation. It would be nice if it was possible to create a <.text+offset> expression for the AUIPC instruction but I just don't know how. Otherwise I have also tried splitting up the instruction earlier, but the AUIPC/ADDI get split up too often to make it feasible. Also there's no real benefit because RISCVMergeBaseOffset cannot work on the %pcrel_lo base symbols without a great deal of modification. @rogfer01 what do you do differently to this patch for the PC-relative case?

In D55296#1319274, @efriedma wrote:

Okay, then LGTM

In D55279#1319007, @rogfer01 wrote:

Ah, thanks @asb definitely I was looking at the wrong place!

Ah, never mind, D54421 was updated to also patch lld at the same time, so this has been merged already.

Ping? Stack is now empty.

In D54143#1305469, @rogfer01 wrote:

In my downstream I have a single RISCVISD::WRAPPER_PIC and then I use a target flag to remember whether later we want just PCREL (lla) or GOT based on what shouldAssumeDSOLocal returns.

But I agree that we can do instead what @jrtc27 suggests and then we can add a PseudoLA later (or something that explicitly means "via GOT" because a "la" with -fno-PIC is just a lla),