Page MenuHomePhabricator

m68k: Support bit shifts on 64-bit integers
ClosedPublic

Authored by 0x59616e on Oct 9 2021, 10:25 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
AnnikaCodes requested review of this revision.Oct 9 2021, 10:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2021, 10:25 AM
jrtc27 added inline comments.Oct 9 2021, 10:27 AM
llvm/test/CodeGen/M68k/Arith/bitwise.ll
235

Regenerate the CHECK lines

Regenerate CHECKs

AnnikaCodes added inline comments.Oct 9 2021, 11:39 AM
llvm/test/CodeGen/M68k/Arith/bitwise.ll
235

OK.

jrtc27 added inline comments.Oct 9 2021, 11:46 AM
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).

Use RISC-V's custom lowering

Use RISC-V's custom lowering

(oops I used arc wrong, sorry)

AnnikaCodes added inline comments.Oct 9 2021, 1:55 PM
llvm/test/CodeGen/M68k/Arith/bitwise.ll
241

Yeah, the custom lowering does away with the call.

Fix formatting

3 Updating D111497: m68k: Support bit shifts on 64-bit integers

ricky26 added a subscriber: ricky26.Oct 9 2021, 5:32 PM

Fix Polly checks

(just needed to rebase; I guess the issue wasn't in my changes)

AnnikaCodes added inline comments.Oct 9 2021, 9:26 PM
llvm/lib/Target/M68k/M68kISelLowering.cpp
3267

These other changes are because I ran clang-format on the file; it's syntactically the same.

myhsu added inline comments.Oct 9 2021, 9:49 PM
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

3267

ditto, please use clang-format-diff.py instead

Properly format code

Fix for loop

ok, I reformatted it; hopefully I did it correctly this time.

jrtc27 added inline comments.Oct 10 2021, 11:49 AM
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 ↗(On Diff #378524)

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?)

  • Remove references to XLen and reorder code
AnnikaCodes added inline comments.Oct 10 2021, 1:58 PM
llvm/lib/Target/M68k/M68kISelLowering.cpp
1330

Oh, sorry; I didn't know that.

myhsu accepted this revision.Oct 19 2021, 8:21 PM

LGTM
Thank you!

llvm/lib/Target/M68k/M68kISelLowering.cpp
3309

Appreciate the explanations here :-)

This revision is now accepted and ready to land.Oct 19 2021, 8:21 PM

Could someone land this?

I'm happy to. Are you ready for this to land @AnnikaCodes?

I'm happy to. Are you ready for this to land @AnnikaCodes?

Yeah, I think it's ready.

@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.

@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.

Run clang-format

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?

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

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

Sure, but that's all for post-commit checks, not pre-commit like the failure here.

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

Sure, but that's all for post-commit checks, not pre-commit like the failure here.

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'm still getting test failures with this updated patch.

Yeah, I'm getting the same failure, so it's reproducible.

Hi! Any news on this?

I've been meaning to take a look since it shouldn't need much, but I've not gotten around to it yet.

Any news on this?

0x59616e added inline comments.
llvm/lib/Target/M68k/M68kISelLowering.cpp
3278–3279

That error seems result from here.

3334–3335

...and here.

0x59616e added a comment.EditedFeb 10 2022, 11:15 PM

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.

craig.topper added inline comments.
llvm/lib/Target/M68k/M68kISelLowering.cpp
3256

"register size - ShAmt" -> "register size - 1 - ShAmt" right?

3267

This sub can be an xor. See D119411

I don't m68k, does it have an xor with immediate instruction?

3284

This temporary array is unnecessary. You can write

return DAG.getMergeValues({Lo, Hi}, DL);
craig.topper added a comment.EditedFeb 11 2022, 12:01 AM

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.

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.

@AnnikaCodes if you agree, I can update the diff so that we can land this ASAP.

@AnnikaCodes if you agree, I can update the diff so that we can land this ASAP.

That would be great!, Thanks!

0x59616e commandeered this revision.Feb 11 2022, 2:23 AM
0x59616e added a reviewer: AnnikaCodes.
0x59616e updated this revision to Diff 407814.Feb 11 2022, 2:24 AM

Address feedback.

0x59616e marked 3 inline comments as done.Feb 11 2022, 2:25 AM

I think it's ok to land this. Any suggestion ?

I think this is fine (although to be honest, I don't really know what I'm doing).

This revision was landed with ongoing or failed builds.Feb 11 2022, 2:12 PM
This revision was automatically updated to reflect the committed changes.