Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 25929 Build 25928: arc lint + arc unit
Event Timeline
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
---|---|---|
1483 | 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. |
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
---|---|---|
1483 | 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). |
lib/Target/RISCV/RISCVInstrInfo.td | ||
---|---|---|
486 | Could you remove this for completeness? Not sure why it's all the way up here. |
lib/Target/RISCV/RISCVInstrInfo.td | ||
---|---|---|
823 | Nitpick: this could be indented by one less space. |
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 | Should strip the unwanted whitespace in this file |
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!
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.
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.