Page MenuHomePhabricator

[RISCV] Add InstAlias definitions for add[w], and, xor, or, sll[w], srl[w], sra[w], slt and sltu with immediate.
ClosedPublic

Authored by kito-cheng on Jul 31 2018, 3:07 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

kito-cheng created this revision.Jul 31 2018, 3:07 AM

Update testcase.

sabuasal added inline comments.Jul 31 2018, 11:53 AM
test/MC/RISCV/rvi-aliases-valid.s
179 ↗(On Diff #158200)

This test should print the canonical version of the instruction, you are printing the Alias.

kito-cheng added inline comments.Jul 31 2018, 6:34 PM
test/MC/RISCV/rvi-aliases-valid.s
179 ↗(On Diff #158200)

Those alias instructions always disassemble to i version in GAS even -M no-aliases is given, my thought is keep the behavior with GAS.

asb added a comment.Aug 1 2018, 8:36 AM

Thanks Kito, I've added a couple of comments. I think the main issue is the pattern for the shift amount immediate, which means invalid assembly is accepted.

lib/Target/RISCV/RISCVInstrInfo.td
562–567 ↗(On Diff #158200)

These should use uimmlog2xlen for the shift amount. When fixing this, could you please also add tests that demonstrate the range checks are correct (adding to rv32i-aliases-invalid.s and rv64i-aliases-invalid.s)?

test/MC/RISCV/rvi-aliases-valid.s
179 ↗(On Diff #158200)

Yes, we should match gas here. Perhaps add a comment to this test like "The following aliases are accepted as input but the canonical form of the instruction will always be printed."

kito-cheng updated this revision to Diff 158713.Aug 2 2018, 3:08 AM
kito-cheng retitled this revision from [RISCV] Add InstAlias definitions for add, and, xor, or, sll, srl, sra, slt and sltu with immediate to [RISCV] Add InstAlias definitions for add[w], and, xor, or, sll[w], srl[w], sra[w], slt and sltu with immediate..

Changes:

  • Add addw, sllw, srlw and sraw including testcase.
  • Add comment in testcase.
kito-cheng marked 4 inline comments as done.Aug 2 2018, 3:09 AM
asb added a comment.Aug 2 2018, 5:58 AM

Thanks Kito. This patch breaks rv32i-invalid.s as the error message for add a1, a2, (a3) changes. Could you adjust that test too please?

It probably makes most sense to use 32 as the shamt in the relevant tests in rv32i-aliases-invalid.s and rv64i-aliases-invalid.s

kito-cheng updated this revision to Diff 158894.Aug 2 2018, 7:52 PM

Changes:

  • Update testcase for rv32i-aliases-invalid.s and rv64i-aliases-invalid.s
  • Fix rv32i-invalid.s
asb accepted this revision.Aug 8 2018, 7:44 AM

Thanks Kito, looks good to me.

This revision is now accepted and ready to land.Aug 8 2018, 7:44 AM
This revision was automatically updated to reflect the committed changes.