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 ↗(On Diff #318109)

Move the comment into the assert message?

161 ↗(On Diff #318109)

DebugLoc DL =

llvm/lib/Target/M68k/M68kExpandPseudo.cpp
275 ↗(On Diff #318109)

A more useful assert message would be nice

llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp
529 ↗(On Diff #318109)

(style) Consistently use auto* for cast/dyn_cast

llvm/lib/Target/M68k/M68kInstrInfo.cpp
737 ↗(On Diff #318109)

(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 ↗(On Diff #318109)

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

103 ↗(On Diff #318109)

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

llvm/lib/Target/M68k/M68kMCInstLower.cpp
119 ↗(On Diff #318109)

(style) Move this after the switch statement

165 ↗(On Diff #318109)

(style) Move this after the switch statement

llvm/lib/Target/M68k/M68kSubtarget.cpp
186 ↗(On Diff #318109)
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 ↗(On Diff #318109)

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

103 ↗(On Diff #318109)

ditto the same warning

llvm/lib/Target/M68k/M68kMCInstLower.cpp
119 ↗(On Diff #318109)

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 ↗(On Diff #318109)

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
16 ↗(On Diff #318903)

capitalize

18 ↗(On Diff #318903)

Is this header necessary?

llvm/lib/Target/M68k/M68kAsmPrinter.cpp
48 ↗(On Diff #318903)

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

llvm/lib/Target/M68k/M68kAsmPrinter.h
24 ↗(On Diff #318903)

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

llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp
37 ↗(On Diff #318903)

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

48 ↗(On Diff #318903)

enum class?

67 ↗(On Diff #318903)

constify?

82 ↗(On Diff #318903)

constify?

90 ↗(On Diff #318903)

constify?

91 ↗(On Diff #318903)

assert message?

125 ↗(On Diff #318903)

constify?

144 ↗(On Diff #318903)

constify?

159 ↗(On Diff #318903)

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

llvm/lib/Target/M68k/M68kFrameLowering.cpp
107 ↗(On Diff #318903)

(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);
108 ↗(On Diff #318903)

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

115 ↗(On Diff #318903)

(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);
117 ↗(On Diff #318903)

drop the else

145 ↗(On Diff #318903)

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

428 ↗(On Diff #318903)

drop this commented out code?

llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp
48 ↗(On Diff #318903)

enum class?

59 ↗(On Diff #318903)

enum class?

134 ↗(On Diff #318903)

auto *RegNode

271 ↗(On Diff #318903)

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

536 ↗(On Diff #318903)

auto *CP

544 ↗(On Diff #318903)

auto *S

552 ↗(On Diff #318903)

auto *BA

572 ↗(On Diff #318903)

auto *G

576 ↗(On Diff #318903)

auto *CP

581 ↗(On Diff #318903)

auto *S

586 ↗(On Diff #318903)

auto *J

589 ↗(On Diff #318903)

auto *BA

699 ↗(On Diff #318903)

use llvm::any_of ?

llvm/lib/Target/M68k/M68kISelLowering.cpp
294 ↗(On Diff #318903)

auto *Ld

951 ↗(On Diff #318903)

commented out code - is this necessary?

1086 ↗(On Diff #318903)

necessary?

1490 ↗(On Diff #318903)

iirc we support this generically through a TLI callback.

1535 ↗(On Diff #318903)

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

1633 ↗(On Diff #318903)

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

1930 ↗(On Diff #318903)

for-range loop

2672 ↗(On Diff #318903)

necessary?

2756 ↗(On Diff #318903)

avoid auto for anything but casts or iterators

2967 ↗(On Diff #318903)

necessary?

3008 ↗(On Diff #318903)

for-range loop?

3557 ↗(On Diff #318903)

necessary?

llvm/lib/Target/M68k/M68kInstrBuilder.h
25 ↗(On Diff #318903)

update the guardname

llvm/lib/Target/M68k/M68kInstrInfo.cpp
104 ↗(On Diff #318903)

avoid auto

108 ↗(On Diff #318903)

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

llvm/lib/Target/M68k/M68kMCInstLower.h
21 ↗(On Diff #318903)

how many of these headers are necessary?

llvm/lib/Target/M68k/M68kTargetObjectFile.h
17 ↗(On Diff #318903)

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
699 ↗(On Diff #318903)

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
1490 ↗(On Diff #318903)

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?

1535 ↗(On Diff #318903)

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
1490 ↗(On Diff #318903)

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
699 ↗(On Diff #318903)

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
1453 ↗(On Diff #320681)

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

1473 ↗(On Diff #320681)

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
1453 ↗(On Diff #320681)

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
15 ↗(On Diff #321309)

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

58 ↗(On Diff #321309)

Ditto

62 ↗(On Diff #321309)

Ditto

llvm/lib/Target/M68k/M68kCallingConv.h
38 ↗(On Diff #321309)

Another GitHub fork issue number

55 ↗(On Diff #321309)

... because?

jrtc27 added inline comments.Feb 7 2021, 1:02 PM
llvm/lib/Target/M68k/M68kExpandPseudo.cpp
256 ↗(On Diff #321309)

Commented-out code

259 ↗(On Diff #321309)

Another GitHub fork issue number

llvm/lib/Target/M68k/M68kFrameLowering.cpp
52 ↗(On Diff #321309)

Another GitHub fork issue number

425 ↗(On Diff #321309)

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

435 ↗(On Diff #321309)

Another GitHub fork issue number

662 ↗(On Diff #321309)

Another GitHub fork issue number

667 ↗(On Diff #321309)

Do you want an assert or llvm_unreachable here?

671–673 ↗(On Diff #321309)

Commented-out code

llvm/lib/Target/M68k/M68kFrameLowering.h
170 ↗(On Diff #321309)

Another GitHub fork issue number

llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp
408 ↗(On Diff #321309)

Another GitHub fork issue number

llvm/lib/Target/M68k/M68kISelLowering.cpp
57–58 ↗(On Diff #321309)

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

162 ↗(On Diff #321309)

Nested comment is poor style

221–222 ↗(On Diff #321309)

/*foo=*/val

228 ↗(On Diff #321309)

Commented-out code and another GitHub fork issue number

411–426 ↗(On Diff #321309)

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

437–440 ↗(On Diff #321309)

Commented-out code and GitHub fork issue number

454–457 ↗(On Diff #321309)

Commented-out code and GitHub fork issue number

506–510 ↗(On Diff #321309)

Commented-out code and GitHub fork issue number

516–528 ↗(On Diff #321309)

Commented-out code and GitHub fork issue number

574 ↗(On Diff #321309)

GitHub fork issue number

678–712 ↗(On Diff #321309)

Commented-out code and GitHub fork issue number

993 ↗(On Diff #321309)

GitHub fork issue number

994 ↗(On Diff #321309)

Inappropriate language

1050 ↗(On Diff #321309)

GitHub fork issue number

1061–1063 ↗(On Diff #321309)

Commented-out code

1171–1182 ↗(On Diff #321309)

Commented-out code

1670–1671 ↗(On Diff #321309)

Commented-out code

2034 ↗(On Diff #321309)

Hashes are meaningless upstream

2050 ↗(On Diff #321309)

Commented-out code

2157 ↗(On Diff #321309)

Commented-out code

2271 ↗(On Diff #321309)

Commented-out code

2342 ↗(On Diff #321309)

[su]mulo are currently not checked

2349–2351 ↗(On Diff #321309)

Commented-out code

2395 ↗(On Diff #321309)

Commented-out code

2534 ↗(On Diff #321309)

Commented-out code

2582 ↗(On Diff #321309)

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
161–164 ↗(On Diff #321309)

Commented-out code

llvm/lib/Target/M68k/M68kInstrInfo.cpp
789 ↗(On Diff #321309)

GitHub fork issue number

llvm/lib/Target/M68k/M68kRegisterInfo.cpp
173–175 ↗(On Diff #321309)

Commented-out code and GitHub fork issue number

llvm/lib/Target/M68k/M68kSubtarget.cpp
74–76 ↗(On Diff #321309)

Commented-out code

llvm/lib/Target/M68k/M68kTargetMachine.cpp
80 ↗(On Diff #321309)

GitHub fork issue number

161 ↗(On Diff #321309)

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
55 ↗(On Diff #321309)

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

llvm/lib/Target/M68k/M68kISelLowering.cpp
217–218 ↗(On Diff #323439)

Still missing = signs for 2/3

llvm/lib/Target/M68k/M68kTargetMachine.cpp
36 ↗(On Diff #302185)

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
55 ↗(On Diff #321309)

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
62 ↗(On Diff #323669)

IsPtr

llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp
67 ↗(On Diff #318903)

Not done

llvm/lib/Target/M68k/M68kExpandPseudo.cpp
166 ↗(On Diff #323669)

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

llvm/lib/Target/M68k/M68kFrameLowering.cpp
230 ↗(On Diff #323669)

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

350 ↗(On Diff #323669)

Register?

353 ↗(On Diff #323669)

Unnecessary parens

386 ↗(On Diff #323669)

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

llvm/lib/Target/M68k/M68kFrameLowering.h
175 ↗(On Diff #323669)

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
2243 ↗(On Diff #323669)

Capitalise

2756 ↗(On Diff #323669)

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.

2765 ↗(On Diff #323669)

Capitalise all the names of the MBB variables here.

3133–3138 ↗(On Diff #323669)

unless clang-format disagrees in its infinite wisdom

llvm/lib/Target/M68k/M68kISelLowering.h
186 ↗(On Diff #323669)

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
350 ↗(On Diff #323669)

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

llvm/lib/Target/M68k/M68kISelLowering.cpp
2758–2763 ↗(On Diff #327028)

This comment didn't get updated for the capitalised names

2866 ↗(On Diff #327028)

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

llvm/lib/Target/M68k/M68kInstrInfo.h
34 ↗(On Diff #327028)

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
34 ↗(On Diff #328389)

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.
llvm/lib/Target/M680x0/M680x0ISelLowering.h