[PATCH] D88391: [M68k] (Patch 5/8) Target lowering
Simon Pilgrim via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 27 15:22:19 PST 2021
RKSimon added a comment.
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!
================
Comment at: llvm/lib/Target/M68k/M68k.h:16
+#ifndef LLVM_LIB_TARGET_M68k_M68k_H
+#define LLVM_LIB_TARGET_M68k_M68k_H
+
----------------
capitalize
================
Comment at: llvm/lib/Target/M68k/M68k.h:18
+
+#include "llvm/Support/CodeGen.h"
+
----------------
Is this header necessary?
================
Comment at: llvm/lib/Target/M68k/M68kAsmPrinter.cpp:48
+#include "llvm/Target/TargetOptions.h"
+#include <memory>
+
----------------
are all these headers necessary? there's barely anything in the source file.
================
Comment at: llvm/lib/Target/M68k/M68kAsmPrinter.h:24
+#include "llvm/Target/TargetMachine.h"
+
+namespace llvm {
----------------
This header uses std::move and unique_ptr - please can you add an explicit <memory> and <utility> #includes
================
Comment at: llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp:37
+struct MOVEMState {
+private:
+ MachineBasicBlock::iterator Begin;
----------------
Since you're using private data members - why did you create a struct not a class?
================
Comment at: llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp:48
+
+ enum { None, Load, Store } Type;
+
----------------
enum class?
================
Comment at: llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp:67
+
+ unsigned getBase() {
+ assert(Base);
----------------
constify?
================
Comment at: llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp:82
+
+ unsigned getMask() { return Mask; }
+
----------------
constify?
================
Comment at: llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp:90
+ // You need to call this before Mask update
+ UpdateType classifyUpdateByMask(unsigned Value) {
+ assert(Value);
----------------
constify?
================
Comment at: llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp:91
+ UpdateType classifyUpdateByMask(unsigned Value) {
+ assert(Value);
+
----------------
assert message?
================
Comment at: llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp:125
+
+ int getFinalOffset() {
+ assert(
----------------
constify?
================
Comment at: llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp:144
+ bool isLoad() { return Type == Load; }
+ bool isStore() { return Type == Store; }
+};
----------------
constify?
================
Comment at: llvm/lib/Target/M68k/M68kCollapseMOVEMPass.cpp:159
+
+ void Finish(MachineBasicBlock &MBB, MOVEMState &State) {
+ auto MI = State.begin();
----------------
Some comments explaining the process in this pass would be useful,
================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:107
+ return StackOffset::getFixed(Offset + StackSize);
+ }
+ } else if (TRI->needsStackRealignment(MF)) {
----------------
(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);
```
================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:108
+ }
+ } else if (TRI->needsStackRealignment(MF)) {
+ if (FI < 0) {
----------------
drop the else? The if(TRI->hasBasePointer(MF)) should always return no?
================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:115
+ return StackOffset::getFixed(Offset + StackSize);
+ }
+ // FIXME: #7 Support tail calls
----------------
(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);
```
================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:117
+ // FIXME: #7 Support tail calls
+ } else {
+ if (!HasFP)
----------------
drop the else
================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:145
+
+static unsigned getMOVrrOpcode() { return M68k::MOV32rr; }
+
----------------
Are all these necessary? Is there a plan to extend them? I can't tell if they help cleanup code or not....
================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:428
+ // MBB.erase(PI);
+ // if (!doMergeWithPrevious) MBBI = NI;
+ } else if (Opc == M68k::SUB32ri && PI->getOperand(0).getReg() == StackPtr) {
----------------
drop this commented out code?
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:48
+struct M68kISelAddressMode {
+ enum AddrType {
+ ARI, // Address Register Indirect
----------------
enum class?
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:59
+
+ enum { RegBase, FrameIndexBase } BaseType;
+
----------------
enum class?
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:134
+ return false;
+ if (RegisterSDNode *RegNode =
+ dyn_cast_or_null<RegisterSDNode>(BaseReg.getNode()))
----------------
auto *RegNode
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:271
+
+ return false;
+ }
----------------
(style) don't use if-else-else chain as every block returns - split them into separate if()'s
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:536
+ }
+ } else if (ConstantPoolSDNode *CP = dyn_cast<ConstantPoolSDNode>(N0)) {
+ AM.CP = CP->getConstVal();
----------------
auto *CP
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:544
+ }
+ } else if (ExternalSymbolSDNode *S = dyn_cast<ExternalSymbolSDNode>(N0)) {
+ AM.ES = S->getSymbol();
----------------
auto *S
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:552
+ AM.SymbolFlags = J->getTargetFlags();
+ } else if (BlockAddressSDNode *BA = dyn_cast<BlockAddressSDNode>(N0)) {
+ AM.BlockAddr = BA->getBlockAddress();
----------------
auto *BA
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:572
+ if (N.getOpcode() == M68kISD::Wrapper) {
+ if (GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(N0)) {
+ AM.GV = G->getGlobal();
----------------
auto *G
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:576
+ AM.SymbolFlags = G->getTargetFlags();
+ } else if (ConstantPoolSDNode *CP = dyn_cast<ConstantPoolSDNode>(N0)) {
+ AM.CP = CP->getConstVal();
----------------
auto *CP
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:581
+ AM.SymbolFlags = CP->getTargetFlags();
+ } else if (ExternalSymbolSDNode *S = dyn_cast<ExternalSymbolSDNode>(N0)) {
+ AM.ES = S->getSymbol();
----------------
auto *S
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:586
+ AM.MCSym = S->getMCSymbol();
+ } else if (JumpTableSDNode *J = dyn_cast<JumpTableSDNode>(N0)) {
+ AM.JT = J->getIndex();
----------------
auto *J
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:589
+ AM.SymbolFlags = J->getTargetFlags();
+ } else if (BlockAddressSDNode *BA = dyn_cast<BlockAddressSDNode>(N0)) {
+ AM.BlockAddr = BA->getBlockAddress();
----------------
auto *BA
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:699
+ if (isAddressBase(N.getOperand(i))) {
+ return true;
+ }
----------------
use llvm::any_of ?
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:294
+ }
+ } else if (LoadSDNode *Ld = dyn_cast<LoadSDNode>(Arg)) {
+ if (Flags.isByVal())
----------------
auto *Ld
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:951
+ // report_fatal_error("M68k interrupts may take one or two arguments");
+ // }
+
----------------
commented out code - is this necessary?
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1086
+ // MMFI->setVarArgsFrameIndex(0xAAAAAAA);
+ // }
+
----------------
necessary?
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1490
+ }
+ }
+
----------------
iirc we support this generically through a TLI callback.
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1535
+ default:
+ llvm_unreachable("Unknown ovf instruction!");
+ case ISD::SADDO:
----------------
won't this fire for smulo/umulo which you've commented out below?
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1633
+ } else if (Op1.getOpcode() == ISD::Constant) {
+ ConstantSDNode *AndRHS = cast<ConstantSDNode>(Op1);
+ uint64_t AndRHSVal = AndRHS->getZExtValue();
----------------
else if (auto *AndRHS = dyn_cast<ConstantSDNode>(Op1)) {
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1930
+ UE = Op.getNode()->use_end();
+ UI != UE; ++UI)
+ if (UI->getOpcode() == ISD::STORE)
----------------
for-range loop
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2672
+ }
+ } */
+ }
----------------
necessary?
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2756
+
+ auto PtrVT = getPointerTy(DAG.getDataLayout());
+ SDValue Result = DAG.getTargetConstantPool(
----------------
avoid auto for anything but casts or iterators
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2967
+ // return !is64Bit;
+ // }
+}
----------------
necessary?
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:3008
+ sEnd = BB->succ_end();
+ sItr != sEnd; ++sItr) {
+ MachineBasicBlock *succ = *sItr;
----------------
for-range loop?
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:3557
+ // }
+ // }
+
----------------
necessary?
================
Comment at: llvm/lib/Target/M68k/M68kInstrBuilder.h:25
+#ifndef LLVM_LIB_TARGET_M6800_M6800INSTRBUILDER_H
+#define LLVM_LIB_TARGET_M6800_M6800INSTRBUILDER_H
+
----------------
update the guardname
================
Comment at: llvm/lib/Target/M68k/M68kInstrInfo.cpp:104
+
+ auto Opcode = iter->getOpcode();
+
----------------
avoid auto
================
Comment at: llvm/lib/Target/M68k/M68kInstrInfo.cpp:108
+ continue;
+ }
+
----------------
(style) lots of unnecessary braces around single lines in this file
================
Comment at: llvm/lib/Target/M68k/M68kMCInstLower.h:21
+#include "llvm/MC/MCAsmInfo.h"
+#include "llvm/Support/Compiler.h"
+
----------------
how many of these headers are necessary?
================
Comment at: llvm/lib/Target/M68k/M68kTargetObjectFile.h:17
+
+#include "M68kTargetMachine.h"
+
----------------
drop this?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88391/new/
https://reviews.llvm.org/D88391
More information about the llvm-commits
mailing list