Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/test/CodeGen/M68k/Arith/bitwise.ll | ||
---|---|---|
235 | Regenerate the CHECK lines |
llvm/test/CodeGen/M68k/Arith/bitwise.ll | ||
---|---|---|
235 | OK. |
llvm/test/CodeGen/M68k/Arith/bitwise.ll | ||
---|---|---|
241 | Hm now why is this being turned into a libcall, I'd expect Expand to inline it. RISC-V has custom lowering you could copy, but that lowering looks pretty generic (just turn XLEN into the width of the first operand). |
llvm/test/CodeGen/M68k/Arith/bitwise.ll | ||
---|---|---|
241 | Yeah, the custom lowering does away with the call. |
llvm/lib/Target/M68k/M68kISelLowering.cpp | ||
---|---|---|
3364 | These other changes are because I ran clang-format on the file; it's syntactically the same. |
llvm/lib/Target/M68k/M68kISelLowering.cpp | ||
---|---|---|
104 | Please remove curly brace if there is only one statement in for-loop. | |
386 | I feel like this is modified by clang-format. Can you format only the part you changed using clang/tools/clang-format/clang-format-diff.py | |
3364 | ditto, please use clang-format-diff.py instead |
llvm/lib/Target/M68k/M68kISelLowering.cpp | ||
---|---|---|
1322 | Keep order in this file consistent with the header | |
1330 | XLEN is a RISC-V-specific term, referring to the size of its X registers | |
llvm/lib/Target/M68k/M68kISelLowering.h | ||
138 | Don't group with LowerOperation, and put these with all the other LowerX in the private section below (maybe in the same order as the LowerOperation switch?) |
llvm/lib/Target/M68k/M68kISelLowering.cpp | ||
---|---|---|
1330 | Oh, sorry; I didn't know that. |
LGTM
Thank you!
llvm/lib/Target/M68k/M68kISelLowering.cpp | ||
---|---|---|
3406 | Appreciate the explanations here :-) |
@AnnikaCodes, I'm seeing test failures from this patch. It looks like it's generating SETCC nodes which don't return i8 (which the M68k requires).
FAIL: LLVM :: CodeGen/M68k/Arith/bitwise.ll (315 of 45926) ******************** TEST 'LLVM :: CodeGen/M68k/Arith/bitwise.ll' FAILED ******************** Script: -- : 'RUN: at line 2'; /home/ricky26/Projects/OSS/m68k/llvm/build-relwithdebinfo/bin/llc < /home/ricky26/Projects/OSS/m68k/llvm/llvm/test/CodeGen/M68k/Arith/bitwise.ll -mtriple=m68k-linux -verify-machineinstrs | /home/ricky26/Projects/OSS/m68k/llvm/build-relwithdebinfo/bin/FileCheck /home/ricky26/Projects/OSS/m68k/llvm/llvm/test/CodeGen/M68k/Arith/bitwise.ll -- Exit Code: 1 Command Output (stderr): -- llc: /home/ricky26/Projects/OSS/m68k/llvm/llvm/lib/Target/M68k/M68kISelLowering.cpp:1914: llvm::SDValue llvm::M68kTargetLowering::LowerSETCC(llvm::SDValue, llvm::SelectionDAG&) const: Assertion `VT == MVT::i8 && "SetCC type must be 8-bit integer"' failed. PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace. Stack dump: 0. Program arguments: /home/ricky26/Projects/OSS/m68k/llvm/build-relwithdebinfo/bin/llc -mtriple=m68k-linux -verify-machineinstrs 1. Running pass 'Function Pass Manager' on module '<stdin>'. 2. Running pass 'M68k DAG->DAG Pattern Instruction Selection' on function '@lshr64' -snip- #8 0x0000557b6ce29aee llvm::M68kTargetLowering::LowerSETCC(llvm::SDValue, llvm::SelectionDAG&) const /home/ricky26/Projects/OSS/m68k/llvm/llvm/lib/Target/M68k/M68kISelLowering.cpp:1976:1 #9 0x0000557b6ce2a251 llvm::M68kTargetLowering::LowerSELECT(llvm::SDValue, llvm::SelectionDAG&) const /home/ricky26/Projects/OSS/m68k/llvm/llvm/lib/Target/M68k/M68kISelLowering.cpp:2039:5 #10 0x0000557b6dde95a0 (anonymous namespace)::SelectionDAGLegalize::LegalizeOp(llvm::SDNode*) /home/ricky26/Projects/OSS/m68k/llvm/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1281:43
I checked the debug output:
Legalizing: t24: i32,i32 = srl_parts t5, t3, t9 Trying custom legalization Creating constant: t25: i32 = Constant<0> Creating constant: t26: i32 = Constant<1> Creating constant: t27: i32 = Constant<-32> Creating constant: t28: i32 = Constant<31> Creating new node: t29: i32 = add t9, Constant:i32<-32> Creating new node: t30: i32 = sub Constant:i32<31>, t9 Creating new node: t31: i32 = srl t5, t9 Creating new node: t32: i32 = shl t3, Constant:i32<1> Creating new node: t33: i32 = shl t32, t30 Creating new node: t34: i32 = or t31, t33 Creating new node: t35: i32 = srl t3, t9 Creating new node: t36: i32 = srl t3, t29 Creating new node: t38: i32 = setcc t29, Constant:i32<0>, setlt:ch Creating new node: t39: i32 = select t38, t34, t36 Creating new node: t40: i32 = select t38, t35, Constant:i32<0> Creating new node: t41: i32,i32 = merge_values t39, t40 Successfully custom legalized node ... replacing: t24: i32,i32 = srl_parts t5, t3, t9 with: t41: i32,i32 = merge_values t39, t40 and: t41: i32,i32 = merge_values t39, t40
If I understand correctly, I think the only issue is that t38 returns i32 instead of i8.
It might be as simple as making it so that the getSetCC calls use MVT::i8 instead of the input VT.
That sounds right. On RISC-V, XLenVT is the only legal integer type, and thus what is used by getSetCCResultType (ignoring vectorisation), whereas M68k's returns i8, something I missed when outlining how to generalise the RISC-V implementation.
To be honest here, I have no idea what this build error Harbormaster is giving me means.
FAIL: libarcher :: races/task-dependency.c (117 of 305) ******************** TEST 'libarcher :: races/task-dependency.c' FAILED ******************** Script: -- : 'RUN: at line 13'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/task-dependency.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/deflake.bash /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/task-dependency.c : 'RUN: at line 14'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/task-dependency.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp -latomic && env ARCHER_OPTIONS="ignore_serial=1 report_data_leak=1" env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/deflake.bash /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/task-dependency.c -- Exit Code: 1
That's probably https://github.com/llvm/llvm-project/commit/26b675d65eb2d1f117a39eee965652f590373232. It's in unrelated code so you can ignore it, and the buildbot probably isn't even enabling the experimental M68k backend?
FWIW, the M68k backend is actually tested on a different buildbot: http://lab.llvm.org:8014/#/builders/180
OK, I guess it's safe to commit then.
Could anyone with commit access go ahead and push the changes, please?
I'm still getting test failures with this updated patch.
******************** TEST 'LLVM :: CodeGen/M68k/Arith/bitwise.ll' FAILED ******************** Script: -- : 'RUN: at line 2'; /home/ricky26/Projects/OSS/m68k/llvm/build-relwithdebinfo/bin/llc < /home/ricky26/Projects/OSS/m68k/llvm/llvm/test/CodeGen/M68k/Arith/bitwise.ll -mtriple=m68k-linux -verify-machineinstrs | /home/ricky26/Projects/OSS/m68k/llvm/build-relwithdebinfo/bin/FileCheck /home/ricky26/Projects/OSS/m68k/llvm/llvm/test/CodeGen/M68k/Arith/bitwise.ll -- Exit Code: 1 Command Output (stderr): -- llc: /home/ricky26/Projects/OSS/m68k/llvm/llvm/lib/Target/M68k/M68kInstrInfo.cpp:762: virtual void llvm::M68kInstrInfo::storeRegToStackSlot(llvm::MachineBasicBlock&, llvm::MachineBasicBlock::iterator, llvm::Register, bool, int, const llvm::TargetRegisterClass*, const llvm::TargetRegisterInfo*) const: Assertion `MF.getFrameInfo().getObjectSize(FrameIndex) == 4 && "Stack slot too small for store"' failed. PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace. Stack dump: 0. Program arguments: /home/ricky26/Projects/OSS/m68k/llvm/build-relwithdebinfo/bin/llc -mtriple=m68k-linux -verify-machineinstrs 1. Running pass 'Function Pass Manager' on module '<stdin>'. 2. Running pass 'Greedy Register Allocator' on function '@lshr64' -snip- #8 0x0000556e49e37621 llvm::MachineFrameInfo::getObjectSize(int) const /home/ricky26/Projects/OSS/m68k/llvm/llvm/lib/Target/M68k/M68kInstrInfo.cpp:762:3 #9 0x0000556e49e37621 llvm::M68kInstrInfo::storeRegToStackSlot(llvm::MachineBasicBlock&, llvm::MachineInstrBundleIterator<llvm::MachineInstr, false>, llvm::Register, bool, int, llvm::TargetRegisterClass const*, llvm::TargetRegisterInfo const*) const /home/ricky26/Projects/OSS/m68k/llvm/llvm/lib/Target/M68k/M68kInstrInfo.cpp:762:3 #10 0x0000556e4a5c6de4 llvm::TargetInstrInfo::foldMemoryOperand(llvm::MachineInstr&, llvm::ArrayRef<unsigned int>, int, llvm::LiveIntervals*, llvm::VirtRegMap*) const /home/ricky26/Projects/OSS/m68k/llvm/llvm/lib/CodeGen/TargetInstrInfo.cpp:646:24
Debug logging:
selectOrSplit CCRC:%12 [80r,288r:0) 0@80r weight:3.322368e-03 w=3.322368e-03 hints: $ccr RS_Split Cascade 0 Analyze counted 2 instrs in 2 blocks, through 2 blocks. Compact region bundles, v=2, none. Cost of isolating all blocks = 2.0 $ccr static = 1.0, v=2 no bundles. Inline spilling CCRC:%12 [80r,288r:0) 0@80r weight:3.322368e-03 From original %12 Merged spilled regs: SS#0 [80r,288r:0) 0@x weight:0.000000e+00 spillAroundUses %12
It looks like it's trying to spill the CCR, perhaps we can't handle smaller than 32-bit stack slots either? Ideally we shouldn't spill the CCR at all since it's not supported on the 68000, but that's a problem for later.
I've been meaning to take a look since it shouldn't need much, but I've not gotten around to it yet.
the machine ir after dag-isel (-print-after=amdgpu-isel) has a weird COPY:
... %12:ccrc = COPY $ccr ...
I believe this is the root cause.
But that COPY isn't there before dag-combine2 (-view-dag-combine2-dags).
I don't know where it comes from.
llvm/lib/Target/M68k/M68kISelLowering.cpp | ||
---|---|---|
3353 | "register size - ShAmt" -> "register size - 1 - ShAmt" right? | |
3364 | This sub can be an xor. See D119411 I don't m68k, does it have an xor with immediate instruction? | |
3381 | This temporary array is unnecessary. You can write return DAG.getMergeValues({Lo, Hi}, DL); |
To fix the ccrc copy I believe you need to add this to M68kRegisterInfo.cpp
const TargetRegisterClass * M68kRegisterInfo::getCrossCopyRegClass(const TargetRegisterClass *RC) const { if (RC == &M68k::CCRCRegClass) return &M68k::DR32RegClass; return RC; }
I'm not sure if DR32 is the right class. But ultimately you need to tell it that a regular register should be used for copying flags, not the CCRC class. This will convince the scheduler that converts SelectionDAG to MachineIR that it should duplicate the ADD32di that provides that flag for the two CMOV32r instead of trying to copy the flags to the second CMOV32r. This need is needed because the flags are clobbered by other instructions between the first and second CMOV32r.
It works !
My solution seems to be wrong because of the reason Craig just mentioned: flags are clobbered between the two CMOV32r.
Don't group with LowerOperation, and put these with all the other LowerX in the private section below (maybe in the same order as the LowerOperation switch?)