Page MenuHomePhabricator

[RISCV] Add assembler support for LA pseudo-instruction
ClosedPublic

Authored by jrtc27 on Dec 5 2018, 7:18 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jrtc27 created this revision.Dec 5 2018, 7:18 AM
rogfer01 added inline comments.Dec 5 2018, 7:49 AM
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1483 ↗(On Diff #176819)

My understanding is that .option pic and .option nopic change this behaviour.

A quick look at MCObjectFileInfo class files shows that the attribute is only set during the construction of the object and can't be changed.

In my downstream I implemented .option pic and .option nopic which set a field of RISCVAsmParser that I called IsPicEnabled and is only used for la. That field is initialised in the constructor using getContext().getObjectFileInfo()->isPositionIndependent(). Perhaps adding a setter to MCObjectFileInfo might be a better approach.

That said, either we implement .option pic / .option nopic first or we add a FIXME here and implement it later.

Hope this makes sense.

jrtc27 marked an inline comment as done.Dec 5 2018, 8:35 AM
jrtc27 added inline comments.
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1483 ↗(On Diff #176819)

Indeed, you're absolutely right, and I have what sounds like nearly-identical code in my own local repository. Given that, as far as I know, .option pic/nopic only affects the expansion of la, I decided it made sense to add la first rather than a seemingly-pointless option. I'm not sure whether a FIXME is really necessary; anyone adding .option pic/nopic support must look for PIC-related checks, and we have all the code, it's just a case of changing the condition (unlike some cases where FIXMEs do exist in code gen because the expansion just doesn't exist).

rogfer01 marked an inline comment as done.Dec 5 2018, 11:28 AM
rogfer01 added inline comments.
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1483 ↗(On Diff #176819)

Agreed. The FIXME may not help much here. Thanks @jrtc27 !

lewis-revill added inline comments.
lib/Target/RISCV/RISCVInstrInfo.td
486 ↗(On Diff #177723)

Could you remove this for completeness? Not sure why it's all the way up here.

lewis-revill added inline comments.Dec 13 2018, 12:21 PM
lib/Target/RISCV/RISCVInstrInfo.td
823 ↗(On Diff #177723)

Nitpick: this could be indented by one less space.

asb added a comment.Dec 20 2018, 7:08 AM

Thanks, this basically looks good to me. Could you please add some tests for la with invalid operands.

Not hugely important, but I do think that adding a FIXME/TODO as Lewis suggests would be worthwhile. It signals to anyone reading the code that they're encountering a known limitation rather than the author failing to consider the requirements of .option pic/nopic.

test/MC/RISCV/rvi-pseudos.s
37 ↗(On Diff #177723)

Should strip the unwanted whitespace in this file

asb added a comment.Jan 22 2019, 11:48 PM

Hi James. Might you have time to address the outstanding (minor) review comments?

Additionally, can you 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

Thanks!

jrtc27 updated this revision to Diff 185322.Feb 5 2019, 8:47 AM

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

Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 8:47 AM
asb accepted this revision.Feb 12 2019, 8:13 AM

Looks good to me - thanks. I like the refactoring to introduce emitAuipcInstPair. Nit: a comment next to its declaration (along the lines of the comments for neighbouring helper declarations) would be useful.

This revision is now accepted and ready to land.Feb 12 2019, 8:13 AM
This revision was automatically updated to reflect the committed changes.