This patch was split from D122918 .
Co-Author: @StephenFan @liaolucy @realqhc
Paths
| Differential D149743
[RISCV][CodeGen] Support Zdinx on RV32 codegen ClosedPublic Authored by sunshaoce on May 3 2023, 6:12 AM.
Details
Diff Detail
Event Timelinesunshaoce added a parent revision: D149665: [RISCV][CodeGen] Support Zdinx on RV64 codegen.May 3 2023, 6:12 AM Comment Actions In D122918, @jrct27 said
I tested it now. It seems that the bug has already been fixed. $ bin/clang -cc1 -triple riscv32 -target-feature +zdinx -S test.c -O3 foo: # @foo .Lfoo$local: .type .Lfoo$local,@function # %bb.0: # %entry addi a3, a0, 2044 sw a1, 2044(a0) sw a2, 4(a3) ret $ bin/clang -cc1 -triple riscv32 -S test.c -O3 foo: # @foo .Lfoo$local: .type .Lfoo$local,@function # %bb.0: # %entry addi a3, a0, 2044 sw a1, 2044(a0) sw a2, 4(a3) ret $ bin/clang -cc1 -triple riscv32 -target-feature +d -S test.c -O3 foo: # @foo .Lfoo$local: .type .Lfoo$local,@function # %bb.0: # %entry fsd fa0, 2044(a0) ret Any suggestions?@jrtc27 Comment Actions
At the IR level (and DAG level) is it actually doing an f64 store? I can’t see how your code would ever work for that case. I also don’t know if unaligned accesses of globalisation needs to work, because if so your code is similarly broken there. Comment Actions
I think returning an f64 load will get bitcasted by the ABI which will convert the load to an i64 load which will go to type legalization. You need to test a load followed by an FP operation like fadd. Comment Actions Update unpackF64OnRV32DSoftABI, do not use RISCVISD::BuildPairF64 on zdinx, but use build_pair + bitcast. $ bin/clang -cc1 -triple riscv32 -target-feature +zdinx -S test.c -O3 foo: # @foo .Lfoo$local: .type .Lfoo$local,@function # %bb.0: # %entry addi a3, a0, 2044 sw a1, 2044(a0) sw a2, 4(a3) ret realqhc added a child revision: D149811: [RISCV][CodeGen] Support Zhinx and Zhinxmin.May 3 2023, 8:07 PM Comment Actions This test fails the verifier define void @foo(ptr nocapture %p, double %d) { entry: %a = fadd double %d, %d %add.ptr = getelementptr inbounds i8, ptr %p, i64 2044 store double %a, ptr %add.ptr, align 8 ret void } This revision now requires changes to proceed.May 4 2023, 3:59 PM
Comment Actions Maybe we shouldn't expand the Pseudo in RISCVExpandPseudoInsts.cpp. llc: /home/liaochunyu/llvm-project/llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:83: virtual bool (anonymous namespace)::RISCVExpandPseudo::runOnMachineFunction(llvm::MachineFunction &): Assertion `OldSize >= NewSize' failed. Maybe we should learn from jrtc27's reference https://github.com/CTSRD-CHERI/llvm-project/pull/635/files Comment Actions
That error means you're missing a let Size = 8 on the pseudo in tablegen. Comment Actions
Here, it looks like there is let isCall = 0, mayLoad = 1, mayStore = 0, Size = 8, isCodeGenOnly = 1 in def PseudoRV32ZdinxLD : Pseudo<(outs GPRPF64:$dst), (ins GPR:$rs1, simm12:$imm12), []>; defm : LdPat<load, PseudoRV32ZdinxLD, f64>; /// Stores let isCall = 0, mayLoad = 0, mayStore = 1, Size = 8, isCodeGenOnly = 1 in def PseudoRV32ZdinxSD : Pseudo<(outs), (ins GPRPF64:$rs2, GPR:$rs1, simm12:$imm12), []>; defm : StPat<store, PseudoRV32ZdinxSD, GPRPF64, f64>; Comment Actions
Maybe I don’t know you meant by one more MI Comment Actions
One more ADDI than before. else { Register TmpReg = MBBI->getOperand(1).getReg(); BuildMI(MBB, MBBI, DL, TII->get(RISCV::ADDI), TmpReg) .add(MBBI->getOperand(1)) .addImm(4); BuildMI(MBB, MBBI, DL, TII->get(RISCV::SW)) .addReg(Hi, getKillRegState(MBBI->getOperand(0).isKill())) .add(MBBI->getOperand(1)) .add(MBBI->getOperand(2)); } but the test fail: define void @foo(ptr nocapture %p, double %d) { entry: %a = fadd double %d, %d %add.ptr = getelementptr inbounds i8, ptr %p, i64 2044 store double %a, ptr %add.ptr, align 8 ret void } Guess it's because of add one more MI. It suddenly occurred to me that might not be a problem with size. Maybe my global didn't handle it well, then has this error,thanks, thanks!
realqhc removed a child revision: D149811: [RISCV][CodeGen] Support Zhinx and Zhinxmin.May 9 2023, 8:23 PM sunshaoce marked an inline comment as done and an inline comment as not done. Comment Actions
sunshaoce marked 2 inline comments as done. Comment Actions
sunshaoce marked an inline comment as done. Comment Actions
This revision is now accepted and ready to land.May 24 2023, 10:21 AM Closed by commit rG8b90f8e04b8d: [RISCV][CodeGen] Support Zdinx on RV32 codegen (authored by sunshaoce). · Explain WhyMay 24 2023, 11:13 PM This revision was automatically updated to reflect the committed changes. Comment Actions I don’t see any tests for loading from (EDIT: Globals are indeed there, but only for position-dependent medlow code, not sure why my search for %lo earlier didn't work) Comment Actions
I added a test for Constant Pool in D151534. If there are any tests that haven't been covered yet, can you help provide them?
Revision Contents
Diff 522609 llvm/docs/RISCVUsage.rst
llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
llvm/lib/Target/RISCV/RISCVInstrInfoD.td
llvm/lib/Target/RISCV/RISCVInstrInfoZfh.td
llvm/test/CodeGen/RISCV/double-arith-strict.ll
llvm/test/CodeGen/RISCV/double-arith.ll
llvm/test/CodeGen/RISCV/double-bitmanip-dagcombines.ll
llvm/test/CodeGen/RISCV/double-br-fcmp.ll
llvm/test/CodeGen/RISCV/double-calling-conv.ll
llvm/test/CodeGen/RISCV/double-convert-strict.ll
llvm/test/CodeGen/RISCV/double-convert.ll
llvm/test/CodeGen/RISCV/double-fcmp-strict.ll
llvm/test/CodeGen/RISCV/double-fcmp.ll
llvm/test/CodeGen/RISCV/double-frem.ll
llvm/test/CodeGen/RISCV/double-imm.ll
llvm/test/CodeGen/RISCV/double-intrinsics-strict.ll
llvm/test/CodeGen/RISCV/double-intrinsics.ll
llvm/test/CodeGen/RISCV/double-isnan.ll
llvm/test/CodeGen/RISCV/double-mem.ll
llvm/test/CodeGen/RISCV/double-previous-failure.ll
llvm/test/CodeGen/RISCV/double-round-conv-sat.ll
llvm/test/CodeGen/RISCV/double-round-conv.ll
llvm/test/CodeGen/RISCV/double-select-fcmp.ll
llvm/test/CodeGen/RISCV/double-select-icmp.ll
llvm/test/CodeGen/RISCV/double-stack-spill-restore.ll
llvm/test/CodeGen/RISCV/half-convert-strict.ll
llvm/test/CodeGen/RISCV/half-convert.ll
llvm/test/CodeGen/RISCV/zdinx-rv32.ll
|
You don't need the getKillRegState call here. Also it's argument is a bool so passing 0 to it was questionable.