[PATCH] D88391: [M68K] (Patch 5/8) Target lowering

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/M680x0/M680x0.h:3
 //
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
Shouldn't the license be correct in the patch that creates this file?


================
Comment at: llvm/lib/Target/M680x0/M680x0CallingConv.h:56
+
+  // SHIT #34 rewrite this
+  // NOTE This is probably wrong
----------------
:)


================
Comment at: llvm/lib/Target/M680x0/M680x0CollapseMOVEMPass.cpp:105
+    UpdateType Type = ClassifyUpdateByMask(M);
+    // assert(Type != Intermixed);
+    if (Type == Intermixed)
----------------
commented out code


================
Comment at: llvm/lib/Target/M680x0/M680x0CollapseMOVEMPass.cpp:132
+  bool UpdateMask(unsigned Value) {
+    assert(Value == (Value & 0xFFFF) && "Mask must fit 16 bit");
+    assert(!(Value & Mask) &&
----------------
isUInt<16>?


================
Comment at: llvm/lib/Target/M680x0/M680x0FrameLowering.cpp:490
+  // Calculate offsets.
+  for (std::vector<CalleeSavedInfo>::const_iterator I = CSI.begin(),
+                                                    E = CSI.end();
----------------
Can this be a range based for loop?


================
Comment at: llvm/lib/Target/M680x0/M680x0ISelDAGToDAG.cpp:47
+/// FIXME #11 move it somewhere
+inline bool isInt(unsigned N, int64_t x) {
+  return N >= 64 ||
----------------
This is already in MathExtras.h


================
Comment at: llvm/lib/Target/M680x0/M680x0ISelDAGToDAG.cpp:394
+  //
+  // This optimization is valid because the GNU TLS model defines that
+  // gs:0 (or fs:0 on M680x0-64) contains its own address.
----------------
commented out code?


================
Comment at: llvm/lib/Target/M680x0/M680x0ISelDAGToDAG.cpp:524
+
+bool M680x0DAGToDAGISel::matchADD(SDValue N, M680x0ISelAddressMode &AM,
+                                  unsigned Depth) {
----------------
I think N here needs to be a reference. This was a bug in the X86 code this was copied from that has since been fixed.


================
Comment at: llvm/lib/Target/M680x0/M680x0ISelDAGToDAG.cpp:555
+
+  N = Handle.getValue();
+  return false;
----------------
This assignment doesn't do anything if N isn't a reference.


================
Comment at: llvm/lib/Target/M680x0/M680x0MCInstLower.cpp:174
+
+    MCOperand Saved = OutMI.getOperand(0);
+    OutMI = MCInst();
----------------
Do these instructions have more than 1 operand before the conversion? If not you can just setOpcode without creating a new MCInst. This looks a lot like how the X86 code used to be before https://reviews.llvm.org/D66570


================
Comment at: llvm/lib/Target/M680x0/M680x0RegisterInfo.cpp:117
+  int Result = getRegisterOrder(Reg, *getRegClass(M680x0::SPILLRegClassID));
+  if (Result < 0) {
+    llvm_unreachable("Cannot determine spill order");
----------------
This would be shorter as an assert I think.


================
Comment at: llvm/lib/Target/M680x0/M680x0Subtarget.h:47
+  // selected as well.
+  bool IsM68000 = false;
+  bool IsM68010 = false;
----------------
Would this be better as a single enum value like the SSELevel in X86?


================
Comment at: llvm/lib/Target/M680x0/M680x0TargetMachine.cpp:109
+
+  std::string CPU = !CPUAttr.hasAttribute(Attribute::None)
+                        ? CPUAttr.getValueAsString().str()
----------------
Use CPUAttr.isValid() ? CPUAttr.getValueAsString() : TargetCPU

Similar for FS

This was changed for all targets a few months ago. It should be slightly faster.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88391/new/

https://reviews.llvm.org/D88391