[PATCH] D88391: [M68k] (Patch 5/8) Target lowering
Jessica Clarke via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Feb 7 13:02:50 PST 2021
jrtc27 added inline comments.
================
Comment at: llvm/lib/Target/M68k/M68kAsmPrinter.cpp:15
+
+// TODO #33 make it print Motorola asm
+
----------------
This refers to your GitHub fork and doesn't belong here
================
Comment at: llvm/lib/Target/M68k/M68kAsmPrinter.cpp:58
+void M68kAsmPrinter::emitFunctionBodyStart() {
+ // TODO #33
+}
----------------
Ditto
================
Comment at: llvm/lib/Target/M68k/M68kAsmPrinter.cpp:62
+void M68kAsmPrinter::emitFunctionBodyEnd() {
+ // TODO #33
+}
----------------
Ditto
================
Comment at: llvm/lib/Target/M68k/M68kCallingConv.h:38
+/// NOTE this function is used to select registers for formal arguments and call
+/// TODO #34 Need to assigne all the pointers first
+inline bool CC_M68k_Any_AssignToReg(unsigned &ValNo, MVT &ValVT, MVT &LocVT,
----------------
Another GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kCallingConv.h:55
+
+ // FIXME: This is probably wrong
+ auto I = CCInfo.F.arg_begin();
----------------
... because?
================
Comment at: llvm/lib/Target/M68k/M68kExpandPseudo.cpp:256
+ llvm_unreachable("RTD is not implemented");
+ // MIB = BuildMI(MBB, MBBI, DL, TII->get(M68k::RTD)).addImm(StackAdj);
+ } else {
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kExpandPseudo.cpp:259
+ // Copy PC from stack to a free address(A0 or A1) register
+ // TODO #38 check if it is really free
+ BuildMI(MBB, MBBI, DL, TII->get(M68k::MOV32aj), M68k::A1)
----------------
Another GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:52
+
+// FIXME #6 not only pushes....
+bool M68kFrameLowering::hasReservedCallFrame(const MachineFunction &MF) const {
----------------
Another GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:425
+
+ // TODO #8 in the original code for M68k Atom uses lea to adjust stack as an
+ // optimization, can be be this applied for M68k?
----------------
Another GitHub fork issue number. But also this comment makes no sense if by Atom you mean the set of X86 processors?
================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:435
+ .addImm(AbsOffset);
+ // FIXME #9 ATM there is no CCR in these inst
+ MI->getOperand(3).setIsDead(); // The CCR implicit def is dead.
----------------
Another GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:662
+
+ // TODO #10 interrupts...
+ // M68k Interrupt handling function cannot assume anything about the
----------------
Another GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:667
+ //
+ // FIXME: Create "cld" instruction only in these cases:
+ // 1. The interrupt handling function uses any of the "rep" instructions.
----------------
Do you want an assert or llvm_unreachable here?
================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.cpp:671-673
+ // if (Fn.getCallingConv() == CallingConv::M68k_INTR)
+ // BuildMI(MBB, MBBI, DL, TII.get(M68k::CLD))
+ // .setMIFlag(MachineInstr::FrameSetup);
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kFrameLowering.h:170
+
+ /// TODO #39
+ /// Order the symbols in the local stack.
----------------
Another GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kISelDAGToDAG.cpp:408
+ if (AM.isPCRelative()) {
+ // FIXME #12 JumpTable and ExternalSymbol address currently don't like
+ // displacements. It isn't very important, but this should be fixed for
----------------
Another GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:57-58
+
+ // TODO: computeRegisterInfo should able to infer this info
+ // ValueTypeActions.setTypeAction(MVT::i64, TypeExpandInteger);
+
----------------
Commented out with a TODO that it should be able to be inferred doesn't make sense?
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:162
+
+ // 2^2 bytes // ??? can it be just 2^1?
+ setMinFunctionAlignment(Align::Constant<2>());
----------------
Nested comment is poor style
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:221-222
+ Chain, DL, Dst, Src, SizeNode, Flags.getNonZeroByValAlign(),
+ /*isVolatile*/ false, /*AlwaysInline=*/true,
+ /*isTailCall*/ false, MachinePointerInfo(), MachinePointerInfo());
+}
----------------
`/*foo=*/val`
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:228
+ return false;
+ // return CC == CallingConv::Fast; // TODO #7 Since M68010 only
+}
----------------
Commented-out code and another GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:411-426
+ // TODO #10 interrupts
+ // if (CallConv == CallingConv::M68k_INTR) {
+ // const M68kSubtarget& Subtarget =
+ // static_cast<const M68kSubtarget&>(DAG.getSubtarget());
+ // // M68k interrupts may take one or two arguments.
+ // // On the stack there will be no return address as in regular call.
+ // // Offset of last argument need to be set to -4/-8 bytes.
----------------
Commented-out code and two cases of GitHub fork issue numbers
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:437-440
+ // TODO #10 interrupts
+ // if (CallConv == CallingConv::M68k_INTR) {
+ // MFI.setObjectOffset(FI, Offset);
+ // }
----------------
Commented-out code and GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:454-457
+ // TODO #10 interrupts
+ // if (CallConv == CallingConv::M68k_INTR) {
+ // MFI.setObjectOffset(FI, Offset);
+ // }
----------------
Commented-out code and GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:506-510
+ // const M68kRegisterInfo *TRI = Subtarget.getRegisterInfo();
+
+ // TODO #10 interrupts
+ // if (CallConv == CallingConv::M68k_INTR)
+ // report_fatal_error("M68k interrupts may not be called directly");
----------------
Commented-out code and GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:516-528
+ // FIXME #7 Add tailcalls support
+ // if (Subtarget.isPICStyleGOT() &&
+ // !MF.getTarget().Options.GuaranteedTailCallOpt) {
+ // // If we are using a GOT, disable tail calls to external symbols with
+ // // default visibility. Tail calling such a symbol requires using a GOT
+ // // relocation, which forces early binding of the symbol. This breaks code
+ // // that require lazy function symbol resolution. Using musttail or
----------------
Commented-out code and GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:574
+
+ // TODO #44 debug this:
+ int FPDiff = 0;
----------------
GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:678-712
+ // FIXME #16 Fix PIC style GOT
+ // ??? The only time GOT is really needed is for Medium-PIC static data
+ // ??? otherwise we are happy with pc-rel or static references
+ // if (Subtarget.isPICStyleGOT()) {
+ // // ELF / PIC requires GOT in the %BP register before function calls via
+ // PLT
+ // // GOT pointer.
----------------
Commented-out code and GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:993
+ // If value is passed via pointer - do a load.
+ // TODO #45 debug how this really works
+ // ??? May I remove this indirect shizzle?
----------------
GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:994
+ // TODO #45 debug how this really works
+ // ??? May I remove this indirect shizzle?
+ if (VA.getLocInfo() == CCValAssign::Indirect)
----------------
Inappropriate language
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1050
+ for (ForwardedRegister &F : Forwards) {
+ // FIXME #7 Can we use a less constrained schedule?
+ SDValue RegVal = DAG.getCopyFromReg(Chain, DL, F.VReg, F.VT);
----------------
GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1061-1063
+ // } else if (CCID == CallingConv::M68k_INTR && Ins.size() == 2) {
+ // // M68k interrupts must pop the error code if present
+ // MMFI->setBytesToPopOnReturn(4);
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1171-1182
+ // ??? What is it doing?
+ // const M68kRegisterInfo *TRI = Subtarget.getRegisterInfo();
+ // const MCPhysReg *I =
+ // TRI->getCalleeSavedRegsViaCopy(&DAG.getMachineFunction());
+ // if (I) {
+ // for (; *I; ++I) {
+ // if (M68k::GR64RegClass.contains(*I))
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1670-1671
+ return M68k::COND_NE;
+ // case ISD::SETUO: return M68k::COND_P;
+ // case ISD::SETO: return M68k::COND_NP;
+ case ISD::SETOEQ:
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2034
+ if (isOneConstant(Op1)) {
+ // FIXME: See be15dfa88fb1 and a0f4600f4f0ec
+ ISD::CondCode NewCC = ISD::GlobalISel::getSetCCInverse(CC, true);
----------------
Hashes are meaningless upstream
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2050
+ SDValue CCR = EmitCmp(Op0, Op1, M68kCC, DL, DAG);
+ // CCR = ConvertCmpIfNecessary(CCR, DAG);
+ return DAG.getNode(M68kISD::SETCC, DL, MVT::i8,
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2157
+ DAG.getConstant(1, DL, CmpOp0.getValueType()), CmpOp0);
+ // Cmp = ConvertCmpIfNecessary(Cmp, DAG);
+
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2271
+ if (Cond.getOpcode() == M68kISD::SUB) {
+ // Cond = ConvertCmpIfNecessary(Cond, DAG);
+ unsigned CondCode = cast<ConstantSDNode>(CC)->getZExtValue();
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2342
+ if (Cond.getOpcode() == ISD::SETCC) {
+ // Check for setcc([su]{add,sub,mul}o == 0).
+ if (cast<CondCodeSDNode>(Cond.getOperand(2))->get() == ISD::SETEQ &&
----------------
[su]mulo are currently not checked
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2349-2351
+ Cond.getOperand(0).getOpcode() == ISD::USUBO /*||
+ Cond.getOperand(0).getOpcode() == ISD::SMULO ||
+ Cond.getOperand(0).getOpcode() == ISD::UMULO)*/)) {
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2395
+ CondOpcode == ISD::USUBO || CondOpcode == ISD::SSUBO /*||
+ CondOpcode == ISD::UMULO || CondOpcode == ISD::SMULO*/) {
+ SDValue LHS = Cond.getOperand(0);
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2534
+ }
+ // Cond = ConvertCmpIfNecessary(Cond, DAG);
+ return DAG.getNode(M68kISD::BRCOND, DL, Op.getValueType(), Chain, Dest, CC,
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:2582
+// into MOV32ri.
+SDValue M68kTargetLowering::LowerConstantPool(SDValue Op,
+ SelectionDAG &DAG) const {
----------------
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.
================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.h:161-164
+ // void ReplaceNodeResults(SDNode *N, SmallVectorImpl<SDValue>&Results,
+ // SelectionDAG &DAG) const override;
+
+ // SDValue PerformDAGCombine(SDNode *N, DAGCombinerInfo &DCI) const override;
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kInstrInfo.cpp:789
+///
+/// TODOss #47 Eliminate this and move the code to M68kMachineFunctionInfo.
+unsigned M68kInstrInfo::getGlobalBaseReg(MachineFunction *MF) const {
----------------
GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kRegisterInfo.cpp:173-175
+ // unsigned Opc = MI.getOpcode();
+ // FIXME #7 there is no jmp from mem yet
+ // bool AfterFPPop = Opc == M68k::TAILJMPm || Opc == M68k::TCRETURNmi;
----------------
Commented-out code and GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kSubtarget.cpp:74-76
+ // if (StackAlignOverride)
+ // stackAlignment = StackAlignOverride;
+ // else
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kTargetMachine.cpp:80
+ } else if (CM == CodeModel::Kernel) {
+ // FIXME #31 Kernel afaik is small cm plus some weird binding
+ llvm_unreachable("Kernel code model is not supported");
----------------
GitHub fork issue number
================
Comment at: llvm/lib/Target/M68k/M68kTargetMachine.cpp:161
+ addPass(createM68kCollapseMOVEMPass());
+ // addPass(createM68kConvertMOVToMOVMPass());
+}
----------------
Commented-out code
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88391/new/
https://reviews.llvm.org/D88391
More information about the llvm-commits
mailing list