Page MenuHomePhabricator

jrtc27 (James Clarke)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 4 2017, 12:12 PM (119 w, 2 d)

Recent Activity

Thu, Apr 18

jrtc27 updated the diff for D60785: [ELF] Align file offset for .bss if first section in a PT_LOAD.

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

Thu, Apr 18, 8:52 AM · Restricted Project
jrtc27 added inline comments to D60785: [ELF] Align file offset for .bss if first section in a PT_LOAD.
Thu, Apr 18, 8:20 AM · Restricted Project
jrtc27 added inline comments to D60785: [ELF] Align file offset for .bss if first section in a PT_LOAD.
Thu, Apr 18, 6:00 AM · Restricted Project

Wed, Apr 17

jrtc27 added a comment to D45181: [RISCV] Add diff relocation support for RISC-V.

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.

Wed, Apr 17, 6:55 AM · Restricted Project

Tue, Apr 16

jrtc27 updated the diff for D60785: [ELF] Align file offset for .bss if first section in a PT_LOAD.

Updated affected tests.

Tue, Apr 16, 11:15 AM · Restricted Project
jrtc27 created D60785: [ELF] Align file offset for .bss if first section in a PT_LOAD.
Tue, Apr 16, 10:46 AM · Restricted Project

Sat, Apr 13

jrtc27 added inline comments to D60657: [RISCV] Fix evaluation of %pcrel_lo.
Sat, Apr 13, 3:11 PM · Restricted Project

Wed, Apr 3

Herald added a project to D55303: [RISCV] Add lowering of addressing sequences for PIC: Restricted Project.

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

Wed, Apr 3, 7:31 AM · Restricted Project

Thu, Mar 21

jrtc27 added a comment to D39324: [lld] Support TLS in RISC-V.

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

Thu, Mar 21, 6:06 AM · lld
jrtc27 added a comment to D39323: [lld] Support dynamic linking in RISC-V.

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

Thu, Mar 21, 6:06 AM · Restricted Project, lld

Feb 21 2019

jrtc27 added a comment to D54093: [RISCV] Lower inline asm constraints I, J & K for RISC-V.

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.

Feb 21 2019, 7:15 PM · Restricted Project
jrtc27 added a comment to D58531: [clang] Specify type of pthread_create builtin.

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.

Feb 21 2019, 4:29 PM · Restricted Project
jrtc27 added a reviewer for D58531: [clang] Specify type of pthread_create builtin: rsmith.
Feb 21 2019, 4:17 PM · Restricted Project
jrtc27 created D58531: [clang] Specify type of pthread_create builtin.
Feb 21 2019, 4:13 PM · Restricted Project

Feb 20 2019

jrtc27 added a comment to D57141: [RISCV] Add implied zero offset load/store alias patterns.

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?

Feb 20 2019, 12:44 PM · Restricted Project
jrtc27 retitled D57141: [RISCV] Add implied zero offset load/store alias patterns from [RISCV] Add implied zero offset load/store alias pattern to [RISCV] Add implied zero offset load/store alias patterns.
Feb 20 2019, 12:42 PM · Restricted Project
jrtc27 retitled D57141: [RISCV] Add implied zero offset load/store alias patterns from [RISCV][WIP] Add implied zero offset load/store alias pattern to [RISCV] Add implied zero offset load/store alias pattern.
Feb 20 2019, 12:42 PM · Restricted Project

Feb 19 2019

jrtc27 added a comment to D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics.

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.

Feb 19 2019, 4:31 AM · Restricted Project

Feb 18 2019

jrtc27 added inline comments to D58335: [DebugInfo] Generate fixups as emitting DWARF .debug_frame/.eh_frame..
Feb 18 2019, 11:14 AM · Restricted Project
jrtc27 added a comment to D58335: [DebugInfo] Generate fixups as emitting DWARF .debug_frame/.eh_frame..

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.

Feb 18 2019, 11:06 AM · Restricted Project

Feb 15 2019

jrtc27 added a comment to D54093: [RISCV] Lower inline asm constraints I, J & K for RISC-V.
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.

Feb 15 2019, 5:56 AM · Restricted Project
jrtc27 added inline comments to D50496: [RISCV] Implment pseudo instructions for load/store from a symbol address..
Feb 15 2019, 5:03 AM · Restricted Project

Feb 6 2019

jrtc27 requested changes to D54295: [WIP, RISCV] Add inline asm constraint A for RISC-V.
Feb 6 2019, 7:00 AM · Restricted Project, Restricted Project

Feb 5 2019

jrtc27 added a comment to D54296: [WIP, RISCV] Lower inline asm constraint A for RISC-V.

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.

Feb 5 2019, 4:35 PM · Restricted Project
jrtc27 created D57795: [RISCV] Add FreeBSD targets.
Feb 5 2019, 4:16 PM · Restricted Project
jrtc27 requested changes to D57497: [RISCV] Passing small data limitation value to RISCV backend.

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.

Feb 5 2019, 4:00 PM · Restricted Project
jrtc27 updated the diff for D55277: [RISCV] Match GNU tools canonical JALR and add aliases.

Fixed broken test

Feb 5 2019, 3:15 PM · Restricted Project
jrtc27 updated the diff for D57141: [RISCV] Add implied zero offset load/store alias patterns.

Support all memory instructions; add test coverage

Feb 5 2019, 3:03 PM · Restricted Project
jrtc27 commandeered D57141: [RISCV] Add implied zero offset load/store alias patterns.
Feb 5 2019, 3:02 PM · Restricted Project
jrtc27 added a parent revision for D57792: [RISCV] Support z and i operand modifiers: D54296: [WIP, RISCV] Lower inline asm constraint A for RISC-V.
Feb 5 2019, 2:59 PM · Restricted Project
jrtc27 added a child revision for D54296: [WIP, RISCV] Lower inline asm constraint A for RISC-V: D57792: [RISCV] Support z and i operand modifiers.
Feb 5 2019, 2:59 PM · Restricted Project
jrtc27 created D57792: [RISCV] Support z and i operand modifiers.
Feb 5 2019, 2:58 PM · Restricted Project
jrtc27 added a comment to D57240: [RISCV] Don't incorrectly force relocation for %pcrel_lo.

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.

Feb 5 2019, 10:19 AM · Restricted Project
jrtc27 added inline comments to D55341: [RISCV] Support assembling TLS add and associated modifiers.
Feb 5 2019, 10:14 AM · Restricted Project
jrtc27 requested changes to D55342: [RISCV] Support assembling %tls_{ie,gd}_pcrel_hi modifiers.
Feb 5 2019, 9:57 AM · Restricted Project
jrtc27 requested changes to D55341: [RISCV] Support assembling TLS add and associated modifiers.
Feb 5 2019, 9:52 AM · Restricted Project
Herald added a project to D55667: [RISCV] Support assembling TLS LA pseudo instructions: Restricted Project.

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

Feb 5 2019, 9:42 AM · Restricted Project
jrtc27 added a comment to D55560: [RISCV] Attach VK_RISCV_CALL to symbols upon creation.

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

Feb 5 2019, 9:32 AM · Restricted Project
jrtc27 added a comment to D54143: [RISCV] Generate address sequences suitable for mcmodel=medium.

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

Feb 5 2019, 9:25 AM · Restricted Project
jrtc27 added a comment to D55560: [RISCV] Attach VK_RISCV_CALL to symbols upon creation.

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

Feb 5 2019, 9:22 AM · Restricted Project
jrtc27 added a parent revision for D55560: [RISCV] Attach VK_RISCV_CALL to symbols upon creation: D54143: [RISCV] Generate address sequences suitable for mcmodel=medium.
Feb 5 2019, 9:21 AM · Restricted Project
jrtc27 added a child revision for D54143: [RISCV] Generate address sequences suitable for mcmodel=medium: D55560: [RISCV] Attach VK_RISCV_CALL to symbols upon creation.
Feb 5 2019, 9:21 AM · Restricted Project
jrtc27 updated the diff for D55277: [RISCV] Match GNU tools canonical JALR and add aliases.

Rebased and added TODO comment.

Feb 5 2019, 9:14 AM · Restricted Project
jrtc27 updated the diff for D55325: [RISCV] Add assembler support for LA pseudo-instruction.

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

Feb 5 2019, 8:48 AM · Restricted Project
jrtc27 added a comment to D55279: [RISCV] Support assembling %got_pcrel_hi operator.
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

Feb 5 2019, 8:30 AM · Restricted Project
jrtc27 updated the diff for D55279: [RISCV] Support assembling %got_pcrel_hi operator.

Rebased (sorrt for the delay).

Feb 5 2019, 8:30 AM · Restricted Project
Herald added a project to D57240: [RISCV] Don't incorrectly force relocation for %pcrel_lo: Restricted Project.

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:

Feb 5 2019, 8:25 AM · Restricted Project

Jan 28 2019

jrtc27 requested changes to D57319: [RISCV] Fix failure to parse parenthesized immediates.

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.

Jan 28 2019, 4:55 AM
jrtc27 requested changes to D57320: [RISCV] Allow parsing immediates that use tilde & exclaim.

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?

Jan 28 2019, 4:49 AM

Jan 24 2019

jrtc27 added a comment to D57141: [RISCV] Add implied zero offset load/store alias patterns.

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?

Jan 24 2019, 8:50 AM · Restricted Project

Jan 22 2019

jrtc27 requested changes to D57055: [RISCV] Mark TLS as supported.
Jan 22 2019, 7:44 AM · Restricted Project
jrtc27 added inline comments to D53235: [RISCV] Add RV64F codegen support.
Jan 22 2019, 6:39 AM

Jan 15 2019

jrtc27 added a comment to D50496: [RISCV] Implment pseudo instructions for load/store from a symbol address..

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.

Jan 15 2019, 12:01 PM · Restricted Project

Jan 8 2019

jrtc27 added inline comments to D51206: [Sparc] Add tail call support.
Jan 8 2019, 10:17 AM

Jan 7 2019

jrtc27 added inline comments to D47755: [RISCV] Insert R_RISCV_ALIGN relocation type and Nops for code alignment when linker relaxation enabled.
Jan 7 2019, 3:56 AM

Dec 14 2018

jrtc27 added reviewers for D55326: [Driver] Fix incorrect GNU triplet for PowerPC on SUSE Linux: hfinkel, rsmith.
Dec 14 2018, 8:02 AM · Restricted Project
jrtc27 added a comment to D54029: [RISCV] Properly evaluate fixup_riscv_pcrel_lo12.

Thanks, looks good to me now.

Dec 14 2018, 5:44 AM · Restricted Project

Dec 13 2018

jrtc27 added a comment to D54029: [RISCV] Properly evaluate fixup_riscv_pcrel_lo12.

@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.

Dec 13 2018, 8:55 AM · Restricted Project
jrtc27 added a comment to D54029: [RISCV] Properly evaluate fixup_riscv_pcrel_lo12.
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.

Dec 13 2018, 8:21 AM · Restricted Project
jrtc27 added a comment to D55296: [Support] Fix GNU/kFreeBSD build.

Okay, then LGTM

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

Dec 13 2018, 8:03 AM
jrtc27 added inline comments to D54143: [RISCV] Generate address sequences suitable for mcmodel=medium.
Dec 13 2018, 5:06 AM · Restricted Project
jrtc27 added inline comments to D55303: [RISCV] Add lowering of addressing sequences for PIC.
Dec 13 2018, 5:01 AM · Restricted Project

Dec 12 2018

jrtc27 added inline comments to D55303: [RISCV] Add lowering of addressing sequences for PIC.
Dec 12 2018, 8:36 AM · Restricted Project

Dec 11 2018

jrtc27 removed a parent revision for D55303: [RISCV] Add lowering of addressing sequences for PIC: D55279: [RISCV] Support assembling %got_pcrel_hi operator.
Dec 11 2018, 4:16 PM · Restricted Project
jrtc27 removed a child revision for D55279: [RISCV] Support assembling %got_pcrel_hi operator: D55303: [RISCV] Add lowering of addressing sequences for PIC.
Dec 11 2018, 4:16 PM · Restricted Project
jrtc27 added inline comments to D55560: [RISCV] Attach VK_RISCV_CALL to symbols upon creation.
Dec 11 2018, 9:07 AM · Restricted Project
jrtc27 added a comment to D54143: [RISCV] Generate address sequences suitable for mcmodel=medium.

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.

Dec 11 2018, 8:57 AM · Restricted Project
jrtc27 added a comment to D54143: [RISCV] Generate address sequences suitable for mcmodel=medium.

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.

Dec 11 2018, 8:55 AM · Restricted Project
jrtc27 added a comment to D55303: [RISCV] Add lowering of addressing sequences for PIC.

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

Dec 11 2018, 8:52 AM · Restricted Project
jrtc27 updated the diff for D55325: [RISCV] Add assembler support for LA pseudo-instruction.

Rebased

Dec 11 2018, 8:41 AM · Restricted Project
jrtc27 added a comment to D55279: [RISCV] Support assembling %got_pcrel_hi operator.

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).

Dec 11 2018, 8:39 AM · Restricted Project
jrtc27 updated the diff for D55279: [RISCV] Support assembling %got_pcrel_hi operator.

Rebased on top of latest changes to D54029

Dec 11 2018, 8:35 AM · Restricted Project
jrtc27 added a comment to D55279: [RISCV] Support assembling %got_pcrel_hi operator.

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

Dec 11 2018, 2:49 AM · Restricted Project

Dec 7 2018

jrtc27 added a comment to D55341: [RISCV] Support assembling TLS add and associated modifiers.

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

Dec 7 2018, 8:00 AM · Restricted Project
jrtc27 requested changes to D55342: [RISCV] Support assembling %tls_{ie,gd}_pcrel_hi modifiers.

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

Dec 7 2018, 7:51 AM · Restricted Project
jrtc27 updated the diff for D55279: [RISCV] Support assembling %got_pcrel_hi operator.

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.

Dec 7 2018, 7:41 AM · Restricted Project
jrtc27 added a reviewer for D54029: [RISCV] Properly evaluate fixup_riscv_pcrel_lo12: jrtc27.
Dec 7 2018, 6:35 AM · Restricted Project
jrtc27 added a parent revision for D55342: [RISCV] Support assembling %tls_{ie,gd}_pcrel_hi modifiers: D54029: [RISCV] Properly evaluate fixup_riscv_pcrel_lo12.
Dec 7 2018, 6:33 AM · Restricted Project
jrtc27 added a child revision for D54029: [RISCV] Properly evaluate fixup_riscv_pcrel_lo12: D55342: [RISCV] Support assembling %tls_{ie,gd}_pcrel_hi modifiers.
Dec 7 2018, 6:33 AM · Restricted Project
jrtc27 added a parent revision for D55279: [RISCV] Support assembling %got_pcrel_hi operator: D54029: [RISCV] Properly evaluate fixup_riscv_pcrel_lo12.
Dec 7 2018, 6:32 AM · Restricted Project
jrtc27 added a child revision for D54029: [RISCV] Properly evaluate fixup_riscv_pcrel_lo12: D55279: [RISCV] Support assembling %got_pcrel_hi operator.
Dec 7 2018, 6:32 AM · Restricted Project
jrtc27 requested changes to D55335: [RISCV] Support assembling @plt symbol operands.

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.

Dec 7 2018, 6:30 AM · Restricted Project

Dec 6 2018

jrtc27 added inline comments to D50496: [RISCV] Implment pseudo instructions for load/store from a symbol address..
Dec 6 2018, 5:15 PM · Restricted Project
jrtc27 added a comment to D54143: [RISCV] Generate address sequences suitable for mcmodel=medium.

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.

Dec 6 2018, 4:56 PM · Restricted Project
jrtc27 added a comment to D54143: [RISCV] Generate address sequences suitable for mcmodel=medium.

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?

Dec 6 2018, 3:25 PM · Restricted Project
jrtc27 added inline comments to D54029: [RISCV] Properly evaluate fixup_riscv_pcrel_lo12.
Dec 6 2018, 6:25 AM · Restricted Project
jrtc27 added a comment to D55279: [RISCV] Support assembling %got_pcrel_hi operator.

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.

Dec 6 2018, 5:52 AM · Restricted Project
jrtc27 added a comment to D55279: [RISCV] Support assembling %got_pcrel_hi operator.

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.

Dec 6 2018, 5:47 AM · Restricted Project
jrtc27 added a comment to D55303: [RISCV] Add lowering of addressing sequences for PIC.

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.

Dec 6 2018, 5:36 AM · Restricted Project
jrtc27 added a comment to D54143: [RISCV] Generate address sequences suitable for mcmodel=medium.

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.

Dec 6 2018, 5:28 AM · Restricted Project

Dec 5 2018

jrtc27 added inline comments to D55325: [RISCV] Add assembler support for LA pseudo-instruction.
Dec 5 2018, 8:35 AM · Restricted Project
jrtc27 requested changes to D55326: [Driver] Fix incorrect GNU triplet for PowerPC on SUSE Linux.

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

Dec 5 2018, 7:42 AM · Restricted Project
jrtc27 created D55325: [RISCV] Add assembler support for LA pseudo-instruction.
Dec 5 2018, 7:19 AM · Restricted Project
jrtc27 added a parent revision for D55325: [RISCV] Add assembler support for LA pseudo-instruction: D55279: [RISCV] Support assembling %got_pcrel_hi operator.
Dec 5 2018, 7:19 AM · Restricted Project
jrtc27 added a child revision for D55279: [RISCV] Support assembling %got_pcrel_hi operator: D55325: [RISCV] Add assembler support for LA pseudo-instruction.
Dec 5 2018, 7:19 AM · Restricted Project
jrtc27 added inline comments to D54143: [RISCV] Generate address sequences suitable for mcmodel=medium.
Dec 5 2018, 2:22 AM · Restricted Project

Dec 4 2018

jrtc27 requested changes to D55304: [RISCV] Lower calls through PLT.
Dec 4 2018, 7:51 PM
jrtc27 requested changes to D55306: [RISCV, WIP] Lower TLS addresses using localexec.

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.

Dec 4 2018, 7:15 PM
jrtc27 requested changes to D55305: [RISCV] Add lowering of global TLS addresses.

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.

Dec 4 2018, 7:11 PM
jrtc27 requested changes to D55303: [RISCV] Add lowering of addressing sequences for PIC.

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.

Dec 4 2018, 7:03 PM · Restricted Project