Page MenuHomePhabricator

[RISCV 16/n] Support and tests for a variety of additional LLVM IR constructs
ClosedPublic

Authored by asb on Feb 14 2017, 5:58 AM.

Details

Summary

Previous patches primarily ensured that codegen was possible for the standard RISC-V instructions. However, there are a number of IR inputs that wouldn't be appropriately lowered. This patch both adds test cases and supports lowering for a number of these cases:

  • Improved sext/zext/trunc support
  • Support for setcc variants that don't map directly to RISC-V instructions
  • Lowering mul, and hence support for external symbols
  • addc, adde, subc, sube
  • mulhs, srem, mulhu, urem, udiv, sdiv
  • {srl,sra,shl}_parts
  • Bare select
  • brind
  • br_jt
  • cttlz, cttz, ctpop
  • rotl, rotr
  • BlockAddress

A future patch will fix the TODOs regarding the cases where a register is wasted holding 0.

Diff Detail

Repository
rL LLVM

Event Timeline

asb created this revision.Feb 14 2017, 5:58 AM
Razer6 added a subscriber: Razer6.Feb 14 2017, 6:28 AM
asb updated this revision to Diff 110359.Aug 9 2017, 4:39 AM

Updates:

  • br_jt and tests
  • ctlz, cttz, ctpop and tests
  • rotl, rotr and tests
  • Fix an incorrect seteq pattern (SLTIU vs SLTI)
asb edited the summary of this revision. (Show Details)Aug 9 2017, 4:39 AM
majnemer added inline comments.
lib/Target/RISCV/RISCVISelLowering.cpp
158 ↗(On Diff #110359)

report_fatal_error

asb updated this revision to Diff 111635.Aug 18 2017, 1:57 AM
asb marked an inline comment as done.

Address review comment from @majnemer (thanks!).

asb updated this revision to Diff 112456.Aug 23 2017, 2:28 PM

Update to reflect split of instruction and pattern definitions, as discussed in http://lists.llvm.org/pipermail/llvm-dev/2017-August/116635.html

apazos added a subscriber: apazos.Aug 25 2017, 1:46 PM
apazos added inline comments.
lib/Target/RISCV/RISCVISelLowering.cpp
411 ↗(On Diff #112456)

code standard reminder: another instance of using {} with one line statement.

test/CodeGen/RISCV/sext-zext-trunc.ll
177 ↗(On Diff #112456)

Nothing is being tested here right? if you need to check a sequence you can use CHECK-NEXT

psnobl added a subscriber: psnobl.Sep 8 2017, 12:10 PM
kparzysz added inline comments.
lib/Target/RISCV/RISCVISelLowering.cpp
158 ↗(On Diff #112456)

Please avoid "else" after "return".

reames accepted this revision.Oct 18 2017, 4:51 PM
reames added a subscriber: reames.

LGTM w/identified issues fixed before submission.

lib/Target/RISCV/RISCVISelLowering.cpp
151 ↗(On Diff #112456)

As per previous comment, use early return idiom here. (required, minor)

152 ↗(On Diff #112456)

I think this needs a comment. Why is emitting the symbol twice the right thing to do?

test/CodeGen/RISCV/bswap-ctlz-cttz-ctpop.ll
14 ↗(On Diff #112456)

Please add meaningful check lines so we can see instructions. Using the check generator would be good.

test/CodeGen/RISCV/sext-zext-trunc.ll
224 ↗(On Diff #112456)

Same here

This revision is now accepted and ready to land.Oct 18 2017, 4:51 PM
asb updated this revision to Diff 122079.Nov 8 2017, 6:01 AM
asb marked 5 inline comments as done.
asb edited the summary of this revision. (Show Details)

Updated to address review comments from @reames. Tests are now generated using utils/update_llc_test_checks.py.

asb added inline comments.Nov 8 2017, 6:04 AM
lib/Target/RISCV/RISCVISelLowering.cpp
152 ↗(On Diff #112456)

As always, thanks for the feedback Philip. What comment would you like to see? Emitting two symbols (e.g. the "Hi" and "Lo" part) is common for fixed length ISAs. I want to make the code as accessible as possible, but explaining hi/lo relocations at each instance might be repetitive?

mgrang added a subscriber: mgrang.Nov 15 2017, 4:00 PM
mgrang added inline comments.
lib/Target/RISCV/RISCVISelLowering.cpp
206 ↗(On Diff #122079)

Period after comment.

lib/Target/RISCV/RISCVInstrInfo.td
332 ↗(On Diff #122079)

Period after comment.

apazos added inline comments.Nov 20 2017, 2:32 PM
test/CodeGen/RISCV/bswap-ctlz-cttz-ctpop.ll
2 ↗(On Diff #122079)

Is the 'I' ISA always going to be the only default extension?
I ask because some tests below have soft calls for multiplication and the checks will fail if 'IM' ISA becomes part of the default extensions.

This is also ready to be merged. Can we have it in today?

This revision was automatically updated to reflect the committed changes.
asb marked 2 inline comments as done.
asb added inline comments.Nov 21 2017, 12:12 AM
test/CodeGen/RISCV/bswap-ctlz-cttz-ctpop.ll
2 ↗(On Diff #122079)

We could change LLVM to default to a more featureful RISC-V ISA string, but it's probably helpful to have tests like this that would pick up such a change if it happens unintentionally.