Page MenuHomePhabricator

[M68k] (Patch 5/8) Target lowering
ClosedPublic

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
jrtc27 added inline comments.Feb 7 2021, 1:02 PM
llvm/lib/Target/M68k/M68kExpandPseudo.cpp
257

Commented-out code

260

Another GitHub fork issue number

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

Another GitHub fork issue number

426

Another GitHub fork issue number. But also this comment makes no sense if by Atom you mean the set of X86 processors?

436

Another GitHub fork issue number

663

Another GitHub fork issue number

668

Do you want an assert or llvm_unreachable here?

672–674

Commented-out code

llvm/lib/Target/M68k/M68kFrameLowering.h
171

Another GitHub fork issue number

llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp
409

Another GitHub fork issue number

llvm/lib/Target/M68k/M68kISelLowering.cpp
58–59

Commented out with a TODO that it should be able to be inferred doesn't make sense?

163

Nested comment is poor style

222–223

/*foo=*/val

229

Commented-out code and another GitHub fork issue number

412–427

Commented-out code and two cases of GitHub fork issue numbers

438–441

Commented-out code and GitHub fork issue number

455–458

Commented-out code and GitHub fork issue number

507–511

Commented-out code and GitHub fork issue number

517–529

Commented-out code and GitHub fork issue number

575

GitHub fork issue number

679–713

Commented-out code and GitHub fork issue number

994

GitHub fork issue number

995

Inappropriate language

1051

GitHub fork issue number

1062–1064

Commented-out code

1172–1183

Commented-out code

1671–1672

Commented-out code

2035

Hashes are meaningless upstream

2051

Commented-out code

2158

Commented-out code

2272

Commented-out code

2343

[su]mulo are currently not checked

2350–2352

Commented-out code

2396

Commented-out code

2535

Commented-out code

2583

There's a lot of duplication in all these. You might wish to look at RISCV's getAddr for a more concise way of doing this.

llvm/lib/Target/M68k/M68kISelLowering.h
162–165

Commented-out code

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

GitHub fork issue number

llvm/lib/Target/M68k/M68kRegisterInfo.cpp
174–176

Commented-out code and GitHub fork issue number

llvm/lib/Target/M68k/M68kSubtarget.cpp
75–77

Commented-out code

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

GitHub fork issue number

161

Commented-out code

myhsu updated this revision to Diff 323439.Feb 12 2021, 12:16 PM
myhsu marked 52 inline comments as done.
  • [NFC] Addressed most of the feedbacks
jrtc27 added inline comments.Feb 12 2021, 12:46 PM
llvm/lib/Target/M68k/M68kCallingConv.h
56

Deleting the comment doesn't fix the issue, unless the comment itself was wrong.

llvm/lib/Target/M68k/M68kISelLowering.cpp
218–219

Still missing = signs for 2/3

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

DataLayout is ABI, you can't change it based on the CPU if you want ABI compatibility (with the exception of optionally adding support for additional types).

  • [NFC] Addressed most of the feedbacks

@jrtc27 Is there anything else you still want to get changed or are these the remaining last pieces?

FWIW, I am happy to send some patches with improvements myself if you happen to find more stuff to complain about later. You know, we want this backend for Debian :-).

myhsu updated this revision to Diff 323669.Feb 14 2021, 10:48 PM
myhsu marked 2 inline comments as done.
  • Addressed feedbacks
llvm/lib/Target/M68k/M68kCallingConv.h
56

for this case, the original comment was summarized in the FIXME right before the function

  • Addressed feedbacks

Any chance we can get this reviewed?

Any chance we can get this reviewed?

We still have pending reviews on other patches of the series from @jrtc27. Let's get all of them solved before we can approve this final one.

Jessica, since you're the last reviewer to approve the series, would you mind approving the patches that you are already happy with? This will make it easier to know which ones are pending review and which ones are already good to go.

Thanks!

@jrtc27 Do you have any more feedback before I accept this and we let the experimental backend be pushed to trunk?

@jrtc27 Do you have any more feedback before I accept this and we let the experimental backend be pushed to trunk?

I would say move forward and land the backend so we can start working on the individual umbrella bugs [1].

[1] https://bugs.llvm.org/show_bug.cgi?id=48791

jrtc27 added inline comments.Feb 28 2021, 8:06 AM
llvm/lib/Target/M68k/M68kCallingConv.h
63

IsPtr

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

Not done

llvm/lib/Target/M68k/M68kExpandPseudo.cpp
167

In a bunch of places. Plus all these kinds of parameters need a capital I.

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

Capitalise (pointing out as it's one of the few instances that's not of the form bool isX)

351

Register?

354

Unnecessary parens

387

DoMergeWithPrevious, or better just MergeWithPrevious, the Do is superfluous.

llvm/lib/Target/M68k/M68kFrameLowering.h
176

Commented-out code. TODO probably belongs in the implementation not the header file, too, even if it does relate to overriding a specific hook.

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

Capitalise

2757

Style, but also bad name (everything's LLVM, what you mean is that it's an IR BB not a Machine BB); BB vs MBB is the usual way to distinguish between the two.

2766

Capitalise all the names of the MBB variables here.

3134–3139

unless clang-format disagrees in its infinite wisdom

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

Use a better name (and capitalise)

myhsu updated this revision to Diff 327028.Feb 28 2021, 10:26 PM
myhsu marked 18 inline comments as done.

[NFC] Addressed feedbacks

jrtc27 added a comment.Mar 3 2021, 6:22 AM

Still a lot of instances of bool isFoo variables/arguments that should be bool IsFoo.

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

I meant the type, not changing the name. Should avoid the need to cast.

llvm/lib/Target/M68k/M68kISelLowering.cpp
2759–2764

This comment didn't get updated for the capitalised names

2867

Does any of this entire comment block apply to m68k? The assembly is X86 SSE...

llvm/lib/Target/M68k/M68kInstrInfo.h
35

Include the argument name

myhsu updated this revision to Diff 328389.Mar 4 2021, 10:00 PM
myhsu marked 4 inline comments as done.

[NFC] Fix formatting

jrtc27 added inline comments.Mar 7 2021, 7:27 AM
llvm/lib/Target/M68k/M68kInstrInfo.h
35

Traditionally written with a lowercase C (and that's what your TableGen backend does, too)

myhsu updated this revision to Diff 328886.Mar 7 2021, 10:48 AM
myhsu marked an inline comment as done.

[NFC] Fix formatting

jrtc27 accepted this revision.Mar 7 2021, 11:39 AM
This revision is now accepted and ready to land.Mar 7 2021, 11:39 AM

Woohoo, finally \o/. Min: Time to push, I guess :D :D :D.

myhsu added a comment.Mar 7 2021, 4:41 PM

Woohoo, finally \o/. Min: Time to push, I guess :D :D :D.

Alright, I've made sure everything is in good shape on the ToT and massaged the commit messages on my side. I'll wait to see if there is any comment until 12:00pm, March 8th PST before pushing them :-)

jrtc27 added a comment.Mar 7 2021, 6:42 PM

@jrtc27 Do you have any more feedback before I accept this and we let the experimental backend be pushed to trunk?

Do you want to officially sign off on this now?

@jrtc27 Do you have any more feedback before I accept this and we let the experimental backend be pushed to trunk?

Do you want to officially sign off on this now?

I don't think I'm official in anyway..... But I think we have a general thumbs up now for committing the patch series and moving further development into trunk as an experimental target.

Kudos to everyone involved.

rengolin accepted this revision.Mar 8 2021, 2:05 AM

Kudos to everyone involved.

Yes! Thank you everyone for the thorough reviews and prompt changes.

This will remain as a great example on how to upstream a back-end.

This revision was landed with ongoing or failed builds.Mar 8 2021, 12:34 PM
This revision was automatically updated to reflect the committed changes.