This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Don't allow i64 vector div by constant to use mulh with Zve64x
ClosedPublic

Authored by eopXD on Jan 22 2022, 12:47 AM.

Details

Summary

EEW=64 of mulh and its vairants requires V extension.

Authored by: Craig Topper <craig.topper@sifive.com> @craig.topper

Diff Detail

Event Timeline

eopXD created this revision.Jan 22 2022, 12:47 AM
eopXD requested review of this revision.Jan 22 2022, 12:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2022, 12:47 AM
craig.topper added inline comments.Jan 22 2022, 12:45 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
919–923

This one should say vXi64. This is the fixed vector section.

llvm/test/CodeGen/RISCV/rvv/vdiv-sdnode-rv32.ll
1 ↗(On Diff #402189)

Can we do this without splitting the tests back into rv32/rv64 versions?

Why do we need need UTC_ARGS: --force-update

eopXD added inline comments.Jan 23 2022, 9:54 PM
llvm/test/CodeGen/RISCV/rvv/vdiv-sdnode-rv32.ll
1 ↗(On Diff #402189)

Can we do this without splitting the tests back into rv32/rv64 versions?

I looked into the test case. Looks like under rv64 and V it loads the argument using two instructions (one to load %hi and one to load %lo). I tried specifying TargetABI lp64 for it but it didn't change.

https://pastebin.com/vrkajMSN

What argument should I add to eliminate this situation? My understanding is that the differences are in XLEN (rv32 vs rv64) and extensions (zve vs v). The machine instruction generated should only differ between different XLEN.

craig.topper added inline comments.Jan 23 2022, 10:02 PM
llvm/test/CodeGen/RISCV/rvv/vdiv-sdnode-rv32.ll
1 ↗(On Diff #402189)

It can't be fixed. There aren't enough bits in the GPR for RV32. We'll just need to have multiple check-prefixes. Something like

CHECK,RV32,RV32-ZVE32
CHECK,RV32,RV32-V
CHECK,RV64,RV64-ZVE32
CHECK,RV64,RV64-V

eopXD added inline comments.Jan 23 2022, 10:05 PM
llvm/test/CodeGen/RISCV/rvv/vdiv-sdnode-rv32.ll
1 ↗(On Diff #402189)

Okay. I will do that and have a single file for an instruction.

eopXD updated this revision to Diff 402398.Jan 23 2022, 10:24 PM

Update test case.

craig.topper added inline comments.Jan 23 2022, 10:31 PM
llvm/test/CodeGen/RISCV/rvv/vdiv-sdnode.ll
3

Did you run the make check-llvm after running the script? FileCheck doesn't like prefixes to appear in check-prefixes if it doesn't appear in the test file. RV32-ZVE64X does not appear on any lines.

eopXD updated this revision to Diff 402399.Jan 23 2022, 10:31 PM

Update comment.

eopXD updated this revision to Diff 402400.Jan 23 2022, 10:35 PM

Update test case.

eopXD marked 2 inline comments as done.Jan 23 2022, 10:45 PM

Concluding the test case change:

  • For eew < 64, check lines should be the same between all 4 lines of RUN:
  • For eew = 64
    • Since this patch rules out vmulh from Zve64*, ZVE64X lines don't use them.
    • GPR length for rv32 and rv64 differ, so ways to load from function arguments differ between RV32-V and RV64-V
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
919–923

Updated.

llvm/test/CodeGen/RISCV/rvv/vdiv-sdnode.ll
3

Updated. Thank you.

frasercrmck added inline comments.Jan 24 2022, 2:20 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
617

Do we need to move this comment down? It still applies at its previous location, doesn't it?

eopXD updated this revision to Diff 402566.Jan 24 2022, 9:23 AM
eopXD marked 3 inline comments as done.

Undo comment moved and follow clang-format.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
617

Yes you are correct. Change reverted.

eopXD edited the summary of this revision. (Show Details)Jan 24 2022, 9:23 AM
This revision is now accepted and ready to land.Jan 24 2022, 10:05 AM
eopXD updated this revision to Diff 402750.Jan 24 2022, 7:40 PM

Rebase with no change, the buildbot halted.

This revision was landed with ongoing or failed builds.Jan 25 2022, 9:55 AM
This revision was automatically updated to reflect the committed changes.