- TargetMachine implementation for M68k
- ISel, ISched for M68k
- Other lowering (e.g. FrameLowering)
- AsmPrinter
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp | ||
---|---|---|
128 | Move the comment into the assert message? | |
162 | DebugLoc DL = | |
llvm/lib/Target/M68k/M68kExpandPseudo.cpp | ||
276 | A more useful assert message would be nice | |
llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp | ||
530 | (style) Consistently use auto* for cast/dyn_cast | |
llvm/lib/Target/M68k/M68kInstrInfo.cpp | ||
738 | (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 | ||
65 | (style) Remove default case and move llvm_unreachable after the switch statement | |
104 | (style) Remove default case and move llvm_unreachable after the switch statement | |
llvm/lib/Target/M68k/M68kMCInstLower.cpp | ||
120 | (style) Move this after the switch statement | |
166 | (style) Move this after the switch statement | |
llvm/lib/Target/M68k/M68kSubtarget.cpp | ||
187 | if (isPositionIndependent()) return M68kII::MO_GOTPCREL; return M68kII::MO_PC_RELATIVE_ADDRESS; |
llvm/lib/Target/M68k/M68kInstrInfo.h | ||
---|---|---|
65 | strange...GCC 9 will still pop up warning (about cases not handled) after I move llvm_unreachable after the switch statement | |
104 | ditto the same warning | |
llvm/lib/Target/M68k/M68kMCInstLower.cpp | ||
120 | GCC 9 popped out warnings about unhandled cases after moving llvm_unreachable after the switch statement |
llvm/lib/Target/M68k/M68kMCInstLower.cpp | ||
---|---|---|
120 | OK - leave it as is. |
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
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
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? |
- 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 |
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. |
- Addressed feedbacks
- Use TargetLowering callback to optimize multiplications instead of doing manually
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. |
- 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.
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 |
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). |
@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 :-).
- Addressed feedbacks
llvm/lib/Target/M68k/M68kCallingConv.h | ||
---|---|---|
56 | for this case, the original comment was summarized in the FIXME right before the function |
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?
I would say move forward and land the backend so we can start working on the individual umbrella bugs [1].
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) |
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 |
llvm/lib/Target/M68k/M68kInstrInfo.h | ||
---|---|---|
35 | Traditionally written with a lowercase C (and that's what your TableGen backend does, too) |
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 :-)
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.
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.
Please can you fix the sorting?