Page MenuHomePhabricator

[M68k] (Patch 5/8) Target lowering
Needs ReviewPublic

Authored by myhsu on Sep 27 2020, 10:20 PM.

Details

Summary
  1. TargetMachine implementation for M68k
  2. ISel, ISched for M68k
  3. Other lowering (e.g. FrameLowering)
  4. AsmPrinter

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
myhsu updated this revision to Diff 295705.Oct 1 2020, 5:24 PM

Update licenses

myhsu updated this revision to Diff 296013.Oct 3 2020, 4:50 PM

Implement the new TargetInstrInfo::isRegisterOperandPCRel interface w/ the help of Beads.

craig.topper added inline comments.Oct 6 2020, 9:36 PM
llvm/lib/Target/M680x0/M680x0.h
3 ↗(On Diff #296013)

Shouldn't the license be correct in the patch that creates this file?

llvm/lib/Target/M680x0/M680x0CallingConv.h
56 ↗(On Diff #296013)

:)

llvm/lib/Target/M680x0/M680x0CollapseMOVEMPass.cpp
105 ↗(On Diff #296013)

commented out code

132 ↗(On Diff #296013)

isUInt<16>?

llvm/lib/Target/M680x0/M680x0FrameLowering.cpp
490 ↗(On Diff #296013)

Can this be a range based for loop?

llvm/lib/Target/M680x0/M680x0ISelDAGToDAG.cpp
47 ↗(On Diff #296013)

This is already in MathExtras.h

394 ↗(On Diff #296013)

commented out code?

524 ↗(On Diff #296013)

I think N here needs to be a reference. This was a bug in the X86 code this was copied from that has since been fixed.

555 ↗(On Diff #296013)

This assignment doesn't do anything if N isn't a reference.

llvm/lib/Target/M680x0/M680x0MCInstLower.cpp
174 ↗(On Diff #296013)

Do these instructions have more than 1 operand before the conversion? If not you can just setOpcode without creating a new MCInst. This looks a lot like how the X86 code used to be before https://reviews.llvm.org/D66570

llvm/lib/Target/M680x0/M680x0RegisterInfo.cpp
117 ↗(On Diff #296013)

This would be shorter as an assert I think.

llvm/lib/Target/M680x0/M680x0Subtarget.h
47 ↗(On Diff #296013)

Would this be better as a single enum value like the SSELevel in X86?

llvm/lib/Target/M680x0/M680x0TargetMachine.cpp
109 ↗(On Diff #296013)

Use CPUAttr.isValid() ? CPUAttr.getValueAsString() : TargetCPU

Similar for FS

This was changed for all targets a few months ago. It should be slightly faster.

myhsu updated this revision to Diff 297734.Oct 12 2020, 6:10 PM
myhsu marked 12 inline comments as done.

Addressed feedbacks

myhsu updated this revision to Diff 297735.Oct 12 2020, 6:18 PM

Minor formatting issues

myhsu marked an inline comment as done.Oct 12 2020, 6:18 PM
myhsu updated this revision to Diff 302185.Nov 1 2020, 4:29 PM
myhsu retitled this revision from [M68K] (Patch 5/8) Target lowering to [M68k] (Patch 5/8) Target lowering.
myhsu edited the summary of this revision. (Show Details)

[NFC] Rename M680x0 to M68k

MaskRay added inline comments.
llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp
56

I only had a very quick overview and this looks fine.

I can't possibly comment on the m68k implementation and I'm hoping other back-end developers with more up-to-date experience will chime in.

llvm/lib/Target/M68k/M68k.h
28

It would be so nice if we could start all new targets with support for Global ISel instead...

Oh well, Long live SelectionDAG! :)

llvm/lib/Target/M68k/M68kSubtarget.cpp
84

Nice!

llvm/lib/Target/M68k/M68kTargetMachine.cpp
36

Are these github issues? If so, it would be nice to replace them with a proper explanation. When moving to the llvm repo, the connection will disappear.

RKSimon added inline comments.Nov 17 2020, 3:56 AM
llvm/lib/Target/M68k/M68k.h
28

Does GlobalISel support big-endian targets?

rengolin added inline comments.Nov 17 2020, 5:36 AM
llvm/lib/Target/M68k/M68k.h
28

Don't know. It was just a tongue-in-cheek comment. :)

myhsu added a comment.Nov 19 2020, 4:50 PM

It would be so nice if we could start all new targets with support for Global ISel instead...

Yeah GlobalISel is definitely in our backlog/TODOs :-) And I think @RKSimon is right, GlobalISel doesn't support big-endian at this time point.

myhsu updated this revision to Diff 306560.Nov 19 2020, 5:04 PM
myhsu marked 6 inline comments as done.
  • Addressed feedbacks
  • Fix some of the build errors generated by previous rebasing
llvm/lib/Target/M68k/M68kTargetMachine.cpp
36

Yes...these numbers were coming from the legacy repo. Will bring some useful context from those issues if there is any

myhsu updated this revision to Diff 309423.Dec 3 2020, 5:49 PM
  • Re-enable analyzeBranch implementation
    • Skip if the branch target is an immediate value, which is rare in real world cases but essential for some of the binary encoding test cases.
myhsu updated this revision to Diff 309460.Dec 3 2020, 11:01 PM
  • Add M68060 sub-target
myhsu updated this revision to Diff 309790.Dec 6 2020, 1:07 PM
  • Fix ISel failure when it comes to 8-bit multiplications

It would be so nice if we could start all new targets with support for Global ISel instead...

Yeah GlobalISel is definitely in our backlog/TODOs :-) And I think @RKSimon is right, GlobalISel doesn't support big-endian at this time point.

Hmm, I am just curious about that whether GlobalISEL is mature enough to use as default instruction selection method without implementing SelectionDAG. As a newer backend, I really would like to drop legacy stuff at start if it works.

llvm/lib/Target/M68k/M68k.h
28

It would be so nice if we could start all new targets with support for Global ISel instead...

Oh well, Long live SelectionDAG! :)

RKSimon added inline comments.Dec 9 2020, 2:24 AM
llvm/lib/Target/M68k/CMakeLists.txt
30

Please can you fix the sorting?

Hmm, I am just curious about that whether GlobalISEL is mature enough to use as default instruction selection method without implementing SelectionDAG. As a newer backend, I really would like to drop legacy stuff at start if it works.

Unfortunately, no. Even if GIsel is mature enough for some codegen (ex. O0 on AArch64), it isn't mature across the board to make SelDAG "legacy" yet.

Waiting for that to happen for your target would be counter productive, especially in an experimental state.

From what I hear, this target is mature enough (using SelDAG) to compile real world applications including possibly Linux.

Replacing SelGDAG with GIsel isn't trivial even if it was mature enough. But as an experimental target, you'll have a lot of other issues to deal with until it moves to production.

So, for now, I'd stick with SelDAG and land the target as is.

myhsu updated this revision to Diff 310753.Dec 9 2020, 8:38 PM
  • [NFC] Fix formatting
myhsu marked an inline comment as done.Dec 9 2020, 8:38 PM

@craig.topper are you happy with the changes?

jrtc27 added inline comments.Dec 20 2020, 11:14 AM
llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp
179

Old name still left

llvm/lib/Target/M68k/M68kISelLowering.h
36

= FIRST_NUMBER?

83

Change of naming style

96

Corresponding LAST_NUMBER?

llvm/lib/Target/M68k/M68kInstrBuilder.h
41

namespace M68k?

llvm/lib/Target/M68k/M68kInstrInfo.cpp
836

These are conventionally all lowercase (otherwise GOT etc would be uppercase too)

llvm/lib/Target/M68k/M68kRegisterInfo.cpp
156–161

?

214–219

?

277–281

?

llvm/lib/Target/M68k/M68kSubtarget.cpp
232
llvm/lib/Target/M68k/M68kTargetMachine.cpp
47–59

As with clang re alignment.

llvm/lib/Target/M68k/M68kTargetObjectFile.cpp
43

Need what?

myhsu updated this revision to Diff 314143.Wed, Dec 30, 12:49 PM
myhsu marked 11 inline comments as done.
  • [NFC] Addressed some of the feedbacks
    • Need to double check the comments related to Clang part changes and pointer size
llvm/lib/Target/M68k/M68kISelLowering.h
36

Looking at other Targets' implementations, I think FIRST_NUMBER should always be ISD::BUILTIN_OP_END instead of the first target-specific ISD type.

83

Well...since X86 has the same naming style for GlobalBaseReg, I'm incline not to change it to be in consistent with X86

96

Taking other Targets as references, I don't think we need to assign LAST_NUMBER here

jrtc27 added inline comments.Wed, Dec 30, 12:57 PM
llvm/lib/Target/M68k/M68kISelLowering.h
83

X86 may be good for seeing what's possible and how to do things, but it's one of the worse backends to look at for style due to its complexity and age.

A few minors - mainly code style concerns, plus a concern about "isM68020()" style function naming.

llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp
1

M68kCollapseMOVEMPass.cpp ?

llvm/lib/Target/M68k/M68kFrameLowering.cpp
147

(style) We don't include function names anymore

192

can we use llvm::any_of here?

850

(style)

for (unsigned i = 0, e = CSI.size(); i < e; ++i) {
870

(style) use a for-range loop?

885

(style) use a for-range loop?

905

(style) use a for-range loop?

917

(style) use a for-range loop?

llvm/lib/Target/M68k/M68kInstrInfo.cpp
366

(style) Avoid using auto

Also - should this be a reference?

369

(style) Avoid using auto

413

(style) Avoid using auto

416

(style) Avoid using auto

llvm/lib/Target/M68k/M68kSubtarget.cpp
165

(style)

if (isPositionIndependent())
   return M68kII::MO_GOTPCREL;

 return M68kII::MO_GOT;
197

(style)

if (isPositionIndependent())
  return M68kII::MO_GOTPCREL;

if (isM68020())
  return M68kII::MO_PC_RELATIVE_ADDRESS;

return M68kII::MO_ABSOLUTE_ADDRESS;
llvm/lib/Target/M68k/M68kSubtarget.h
83

Not sure if the naming here is correct - the function name suggests a specific CPU, but the implementation suggests a minimum CPU feature set.

llvm/lib/Target/M68k/M68kTargetMachine.cpp
4

Add the boilerplate to the initial commit?