This is an archive of the discontinued LLVM Phabricator instance.

[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

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
236

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
449

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

test/CodeGen/RISCV/sext-zext-trunc.ll
178

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
235

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
228

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

229

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
15

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

test/CodeGen/RISCV/sext-zext-trunc.ll
225

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
229

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

Period after comment.

lib/Target/RISCV/RISCVInstrInfo.td
332

Period after comment.

apazos added inline comments.Nov 20 2017, 2:32 PM
test/CodeGen/RISCV/bswap-ctlz-cttz-ctpop.ll
2

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

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.