This is an archive of the discontinued LLVM Phabricator instance.

[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
RKSimon added inline comments.Jan 24 2021, 1:46 PM
llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp
127

Move the comment into the assert message?

161

DebugLoc DL =

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

A more useful assert message would be nice

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

(style) Consistently use auto* for cast/dyn_cast

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

(style)

if (M68k::DR8RegClass.hasSubClassEq(RC))
  return load ? M68k::MOVM8mp_P : M68k::MOVM8pm_P;
if (M68k::CCRCRegClass.hasSubClassEq(RC))
  return load ? M68k::MOV16cp : M68k::MOV16pc;
llvm/lib/Target/M68k/M68kInstrInfo.h
64

(style) Remove default case and move llvm_unreachable after the switch statement

103

(style) Remove default case and move llvm_unreachable after the switch statement

llvm/lib/Target/M68k/M68kMCInstLower.cpp
119

(style) Move this after the switch statement

165

(style) Move this after the switch statement

llvm/lib/Target/M68k/M68kSubtarget.cpp
186
if (isPositionIndependent())
  return M68kII::MO_GOTPCREL;
return M68kII::MO_PC_RELATIVE_ADDRESS;
myhsu updated this revision to Diff 318903.Jan 24 2021, 10:25 PM
myhsu marked 7 inline comments as done.
  • [NFC] Fixed formatting
myhsu added inline comments.Jan 24 2021, 10:26 PM
llvm/lib/Target/M68k/M68kInstrInfo.h
64

strange...GCC 9 will still pop up warning (about cases not handled) after I move llvm_unreachable after the switch statement

103

ditto the same warning

llvm/lib/Target/M68k/M68kMCInstLower.cpp
119

GCC 9 popped out warnings about unhandled cases after moving llvm_unreachable after the switch statement

RKSimon added inline comments.Jan 25 2021, 3:47 AM
llvm/lib/Target/M68k/M68kMCInstLower.cpp
119

OK - leave it as is.

This is the only patch in this series which has not been accepted yet.

This is the only patch in this series which has not been accepted yet.

Yes and no. I had a bunch of comments on many of them and will need to make another pass over them to see if there's anything I've missed (or that's been introduced). All accepted means is that someone in the community thinks it's fine and nobody's explicitly rejected it (which I could have done, but I don't like to use the request changes feature as having big red marks over entire patch series can be demoralising). Certainly shouldn't be landed as soon as everything has an approval, rather that should be the point to request that people who have given feedback take another look over the series if they want to.

Well, it was quick for C-Sky as well which has several patches already landed: https://github.com/llvm/llvm-project/commits/main/llvm/lib/Target/CSKY

Yes and no. I had a bunch of comments on many of them and will need to make another pass over them to see if there's anything I've missed (or that's been introduced). All accepted means is that someone in the community thinks it's fine and nobody's explicitly rejected it (which I could have done, but I don't like to use the request changes feature as having big red marks over entire patch series can be demoralising). Certainly shouldn't be landed as soon as everything has an approval, rather that should be the point to request that people who have given feedback take another look over the series if they want to.

Thank you Jessica, your input and review is excellent and extremely welcome. +1 to everything you said.

Feel free to keep this patch unapproved before you do your final review, to make sure there are no issues left. Thank you!

Well, it was quick for C-Sky as well which has several patches already landed: https://github.com/llvm/llvm-project/commits/main/llvm/lib/Target/CSKY

True, but that was the odd-one-out and we have corrected to conform to the standard for all other back-ends. I explicitly named the m86k upstreaming as the golden rule as to what to do, both from the reviewers and from the authors.

Here's the rationale behind it: https://reviews.llvm.org/D93798#2475025

Well, it was quick for C-Sky as well which has several patches already landed: https://github.com/llvm/llvm-project/commits/main/llvm/lib/Target/CSKY

There's basically nothing of interest there currently.

True, but that was the odd-one-out and we have corrected to conform to the standard for all other back-ends. I explicitly named the m86k upstreaming as the golden rule as to what to do, both from the reviewers and from the authors.

That's very nice to hear.

Here's the rationale behind it: https://reviews.llvm.org/D93798#2475025

Thanks!

a lot of style issues which can summarized by:

  • unnecessary headers - and (vice versa) implicit header dependencies
  • missing assert messages
  • lots of commented out code
  • unnecessary braces around single lines of code
  • using auto too frequently on non-obvious types - lots of consts/pointers/references are missed because of this
  • not consistently using auto* for cast<>/dyn_cast<> - saves a lot of space!
llvm/lib/Target/M68k/M68k.h
17

capitalize

19

Is this header necessary?

llvm/lib/Target/M68k/M68kAsmPrinter.cpp
49

are all these headers necessary? there's barely anything in the source file.

llvm/lib/Target/M68k/M68kAsmPrinter.h
25

This header uses std::move and unique_ptr - please can you add an explicit <memory> and <utility> #includes

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

Since you're using private data members - why did you create a struct not a class?

49

enum class?

68

constify?

83

constify?

91

constify?

92

assert message?

126

constify?

145

constify?

160

Some comments explaining the process in this pass would be useful,

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

(style)

if (FI < 0) {
   // Skip the saved FP.
   return StackOffset::getFixed(Offset + SlotSize);
 }

 assert((-(Offset + StackSize)) % MFI.getObjectAlign(FI).value() == 0);
 return StackOffset::getFixed(Offset + StackSize);
109

drop the else? The if(TRI->hasBasePointer(MF)) should always return no?

116

(style)

if (FI < 0) {
  // Skip the saved FP.
  return StackOffset::getFixed(Offset + SlotSize);
}

assert((-(Offset + StackSize)) % MFI.getObjectAlign(FI).value() == 0);
return StackOffset::getFixed(Offset + StackSize);
118

drop the else

146

Are all these necessary? Is there a plan to extend them? I can't tell if they help cleanup code or not....

429

drop this commented out code?

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

enum class?

60

enum class?

135

auto *RegNode

272

(style) don't use if-else-else chain as every block returns - split them into separate if()'s

537

auto *CP

545

auto *S

553

auto *BA

573

auto *G

577

auto *CP

582

auto *S

587

auto *J

590

auto *BA

700

use llvm::any_of ?

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

auto *Ld

952

commented out code - is this necessary?

1087

necessary?

1491

iirc we support this generically through a TLI callback.

1536

won't this fire for smulo/umulo which you've commented out below?

1634

else if (auto *AndRHS = dyn_cast<ConstantSDNode>(Op1)) {

1931

for-range loop

2673

necessary?

2757

avoid auto for anything but casts or iterators

2968

necessary?

3009

for-range loop?

3558

necessary?

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

update the guardname

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

avoid auto

109

(style) lots of unnecessary braces around single lines in this file

llvm/lib/Target/M68k/M68kMCInstLower.h
22

how many of these headers are necessary?

llvm/lib/Target/M68k/M68kTargetObjectFile.h
18

drop this?

myhsu updated this revision to Diff 320582.Feb 1 2021, 1:37 PM
myhsu marked 47 inline comments as done.
  • Addressed some of the feedbacks
llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp
700

I wish I could...but I don't think SDValue provides any iterator (or begin() / end() / ranges function) over its operands

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

I assume you're talking about the lines above regarding converting mul to shift. If that's the case, are you suggesting. to put those into DAG combiner callbacks?

1536

currently umulo and smulo are legalized by expanding them, so they will not trigger this lowering function.

I'm not sure we should use custom lowering for them though...it seemed like somebody tried before but failed with unknown reason and thus leave the code commented out here

RKSimon added inline comments.Feb 1 2021, 2:52 PM
llvm/lib/Target/M68k/M68kISelLowering.cpp
1491

Yes, you should be able to override TargetLowering::decomposeMulByConstant to let DAGCombine do this for you. See X86TargetLowering::decomposeMulByConstant for an example.

myhsu updated this revision to Diff 320681.Feb 1 2021, 10:52 PM
myhsu marked an inline comment as done.
  • 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.Feb 7 2021, 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?

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.