[PATCH] D60052: Add Connex vector processor back end

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

There's a lot of clutter that makes this hard to review. All of the extra debug, special ifdef blocks, and commented out code should be removed.

This should also be split into smaller patches. For example, the triple patches, then adding the basic target machine definition, and then MC parts before moving on to codegen and optimizations



================
Comment at: include/llvm/ADT/Triple.h:56
     bpfeb,          // eBPF or extended BPF or 64-bit BPF (big endian)
+    connex,         // Connex vector processor
     hexagon,        // Hexagon: hexagon
----------------
The triple patches are usually committed as a first, separate patch


================
Comment at: include/llvm/CodeGen/SelectionDAG.h:273-277
+  DenseMap<const Value*, SDValue> *crtNodeMapPtr;
+
+  void SetNodeMap(DenseMap<const Value *, SDValue> *aCrtNodeMapPtr);
+
+  void UpdateNodeMapSDValue(SDNode *oldSDN, SDValue &newSDV);
----------------
This probably should be dropped


================
Comment at: include/llvm/CodeGen/SelectionDAG.h:1226-1231
+                                SDValue Op1, SDValue Op2,
+                                SDValue Op3, SDValue Op4);
+  MachineSDNode *getMachineNode(unsigned Opcode, const SDLoc &dl, EVT VT1,
+                                EVT VT2, SDValue Op1, SDValue Op2,
+                                SDValue Op3, SDValue Op4);
+  MachineSDNode *getMachineNode(unsigned Opcode, const SDLoc &dl, EVT VT,
----------------
This should be a separate patch (or you could just use the ArrayRef version)


================
Comment at: lib/Target/Connex/ConnexAsmPrinter.cpp:52
+
+#define DEBUG_TYPE "asm-printer"
+
----------------
This should be after all includes


================
Comment at: lib/Target/Connex/ConnexAsmPrinter.cpp:53-58
+
+#include <unordered_map>
+
+#include "ConnexAsmPrinterLoopNests.h"
+
+
----------------
Sort includes


================
Comment at: lib/Target/Connex/ConnexAsmPrinter.cpp:60-63
+// We need to store the correspondence between MachineInstr and the lowered
+//  MCInst, since MCInst does not.
+//  This is used in ConnexInstPrinter.cpp.
+const MachineInstr *crtMI;
----------------
You should not need this, also no global


================
Comment at: lib/Target/Connex/ConnexAsmPrinter.cpp:164
+        //assert(MI2->getOpcode() == TargetOpcode::INLINEASM);
+        if (MI2->getOpcode() == TargetOpcode::INLINEASM) {
+          LLVM_DEBUG(dbgs() << "MoveToFrontRepeat(): Moving the successor "
----------------
isInlineAsm() (also for others)


================
Comment at: lib/Target/Connex/ConnexAsmPrinter.cpp:225-226
+
+            LLVM_DEBUG(dbgs() << " MI->getOperand(" << index << ") = "
+                              << *miOpnd << "\n");
+
----------------
A large number of the debug statements seem like they're just noise for committing


================
Comment at: lib/Target/Connex/ConnexAsmPrinter.cpp:234
+
+              if (strstr(symStr, strToSearch) != NULL) {
+                LLVM_DEBUG(dbgs() << "  MoveToFrontInlineAsm(): Found INLINEASM "
----------------
No c string functions


================
Comment at: lib/Target/Connex/ConnexAsmPrinter.cpp:957
+
+          const char *strPredMBB = predMBB->getName().data();
+
----------------
You shouldn't be looking at the .data() on a StringRef as if it were a c string like this should just use the StringRef directly.


================
Comment at: lib/Target/Connex/ConnexAsmPrinterLoopNests.h:1-2
+//===-- ConnexAsmPrinterLoopNests.h -  -----*- C++ -*--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
I don't really understand anything that's going on in this file. You shouldn't have file IO, or globals 


================
Comment at: lib/Target/Connex/ConnexConfig.h:3
+#define CONNEX_CONFIG_ALEX
+
+// This file is used by ConnexISelDAGToDAG.cpp, ConnexISelLowering.h,
----------------
These macros mostly seem like a waste of effort


================
Comment at: lib/Target/Connex/ConnexHazardRecognizers.cpp:304
+         *
+         * TODO TODO TODO TODO TODO TODO TODO: make full checks and
+         *                 return true only if it
----------------
Fewer TODOs


================
Comment at: lib/Target/Connex/ConnexHazardRecognizers.cpp:308
+         */
+       #ifdef TODO_TODO_TODO_TODO_TODO_TODO_TODO_MORE
+        return true;
----------------
No random special ifdef blocks


================
Comment at: lib/Target/Connex/ConnexInstrInfo.h:52
+
+#ifdef USE_PRERA_HAZARD_RECOGNIZER
+  ScheduleHazardRecognizer *
----------------
No macro for this


================
Comment at: lib/Target/Connex/ConnexSelectionDAGInfo.cpp:113
+
+  char const *FunctionNames[4][3] = {
+    { "__aeabi_memcpy",  "__aeabi_memcpy4",  "__aeabi_memcpy8"  },
----------------
static const


================
Comment at: lib/Target/Connex/ConnexSelectionDAGInfo.cpp:175
+
+  char const *FunctionNames[4][3] = {
+    { "__aeabi_memcpy",  "__aeabi_memcpy4",  "__aeabi_memcpy8"  },
----------------
static const


================
Comment at: lib/Target/Connex/ConnexSelectionDAGInfo.cpp:201
+
+#ifdef NOTNOTNOT
+  const ConnexSubtarget &Subtarget =
----------------
More junk macros


================
Comment at: lib/Target/Connex/ConnexTargetMachine.cpp:898
+  //bool runOnMachineFunction(MachineFunction &MF) override;
+  bool runOnMachineFunction(MachineFunction &MF) {
+    bool changedMF = false;
----------------
This needs to be broken down into smaller functions


================
Comment at: lib/Target/Connex/ConnexTargetTransformInfo.h:117
+        TLI(ST->getTargetLowering()) {
+    LLVM_DEBUG(dbgs() << "Entered constructor ConnexTTIImpl()\n");
+  }
----------------
Noisy debug


================
Comment at: lib/Target/Connex/RecoverFromLlvmIR.h:1-2
+#ifndef RECOVER_FROM_LLVM_IR
+#define RECOVER_FROM_LLVM_IR
+
----------------
I don't know what this file is trying to accomplish, but it is a separate patch from the backend


================
Comment at: lib/Target/Connex/Select_ADDi32_OpincaaCodeGen.h:9-18
+// Code auto-generated by method Kernel::genLLVMISelManualCode().
+//   from Opincaa lib from kernel: add.i32.
+// It is important to put this code in the Select() method of the
+//   SelectionDAGISel class of your back end, after the ISelLowering pass,
+//   which contains the DAG Combiner, because the DAG Combiner can remove
+//   the getCopyToReg() we create, which can lead to the following error:
+//   <<Register use before def!>> assertion failed.
----------------
There shouldn't be any generated code. Generated selection should come from table gen, with some manual code in *ISelDAGToDAG


================
Comment at: lib/Target/Connex/TargetInfo/Makefile:1
+##===- lib/Target/Connex/TargetInfo/Makefile ------------------*- Makefile -*-===##
+#
----------------
I thought Makefiles were all gone?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60052