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 marked an inline comment as done.Feb 1 2021, 10:52 PM
  • Addressed feedbacks
    • Use TargetLowering callback to optimize multiplications instead of doing manually
RKSimon added inline comments.Feb 2 2021, 1:59 AM
llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp
700

You should be able to use SDNode::ops() (or op_begin()/op_end()) - its a SDUse iterator but the SDValue is available through it.

RKSimon added inline comments.Feb 2 2021, 4:50 AM
llvm/lib/Target/M68k/M68kISelLowering.cpp
1454

Why not set Legal/Custom in setOperationAction ISD::MUL based on Subtarget?

1474

Is this i64 lowering active? The action is set to libcall.

myhsu updated this revision to Diff 321309.Feb 3 2021, 9:11 PM
myhsu marked 3 inline comments as done.
  • Addressed feedbacks
llvm/lib/Target/M68k/M68kISelLowering.cpp
1454

yes you're right, this entire function can be replaced by setOperation(LibCall) for i32 and i64 on older sub-architectures.

Any more comments? I'm still seeing some style issues but nothing jumps out as critical.

I also want to make sure that everyone is OK as after this is accepted I see no reason why the whole patch series should not get committed.

Any more comments? I'm still seeing some style issues but nothing jumps out as critical.

I also want to make sure that everyone is OK as after this is accepted I see no reason why the whole patch series should not get committed.

Yes, I fully agree :-). Then we can finally start working on maturing the backend!

jrtc27 added inline comments.Sun, Feb 7, 1:02 PM
llvm/lib/Target/M68k/M68kAsmPrinter.cpp
16

This refers to your GitHub fork and doesn't belong here

59

Ditto

63

Ditto

llvm/lib/Target/M68k/M68kCallingConv.h
39

Another GitHub fork issue number

56

... because?

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.Fri, Feb 12, 12:16 PM
myhsu marked 52 inline comments as done.
  • [NFC] Addressed most of the feedbacks
jrtc27 added inline comments.Fri, Feb 12, 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.Sun, Feb 14, 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.Sun, Feb 28, 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.Sun, Feb 28, 10:26 PM
myhsu marked 18 inline comments as done.

[NFC] Addressed feedbacks

jrtc27 added a comment.Wed, Mar 3, 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.Thu, Mar 4, 10:00 PM
myhsu marked 4 inline comments as done.

[NFC] Fix formatting