[PATCH] D86269: [RFC][Target] Add a new backend target called CSKY

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/IR/CMakeLists.txt:17
 tablegen(LLVM IntrinsicsRISCV.h -gen-intrinsic-enums -intrinsic-prefix=riscv)
+tablegen(LLVM IntrinsicsCSKY.h -gen-intrinsic-enums -intrinsic-prefix=csky)
 tablegen(LLVM IntrinsicsS390.h -gen-intrinsic-enums -intrinsic-prefix=s390)
----------------
I believe this list is in alphabetical order.  So CSKY should be between BBF and Hexagon


================
Comment at: llvm/include/llvm/IR/IntrinsicsCSKY.td:1
+//===- IntrinsicsCSKY.td - Defines CSKY intrinsics -----------*- tablegen -*-===//
+//
----------------
Is this line longer than 80 characters? It looks longer than line 8 which I assume is 80 characters.


================
Comment at: llvm/lib/Target/CSKY/CSKYAsmPrinter.h:1
+//===-- CSKYAsmPrinter.h - CSKY implementation of AsmPrinter ------*- C++
+//-*-===//
----------------
Can we shorten this line to avoid wrapping?

I think the  -*- C++ -*- part needs to stay together.


================
Comment at: llvm/lib/Target/CSKY/CSKYCallingConv.h:1
+//=== CSKYCallingConv.h - CSKY Custom Calling Convention Routines -*- C++
+//-*-===//
----------------
Avoid wrapping this


================
Comment at: llvm/lib/Target/CSKY/CSKYConstantPoolValue.cpp:1
+//===-- CSKYConstantPoolValue.cpp - CSKY constant-pool value --------===//
+//
----------------
Pad to 80 columns


================
Comment at: llvm/lib/Target/CSKY/CSKYConstantPoolValue.h:1
+//===- CSKYConstantPoolValue.h - CSKY constant-pool value -*- C++ -*-===//
+//
----------------
Pad to 80 columns


================
Comment at: llvm/lib/Target/CSKY/CSKYFrameLowering.cpp:27
+                      int NumBytes, unsigned MIFlags = MachineInstr::NoFlags) {
+  bool isSub = NumBytes < 0;
+  unsigned Opc = isSub ? CSKY::SUBI32 : CSKY::ADDI32;
----------------
Capitalize variable name


================
Comment at: llvm/lib/Target/CSKY/CSKYISelDAGToDAG.cpp:94
+
+  auto SDIVREM = CurDAG->getMachineNode(CSKY::DIVF, SDLoc(N), MVT::Untyped,
+                                        {N->getOperand(0), N->getOperand(1)});
----------------
I believe LLVM style would prefer "auto *SDIVREM" over "auto SDIVREM" so it;s clear to the read it is a pointer. This applies to other places in this file.


================
Comment at: llvm/lib/Target/CSKY/CSKYISelLowering.cpp:29
+
+static const uint16_t GPRArgRegs[] = {CSKY::R0, CSKY::R1, CSKY::R2, CSKY::R3};
+
----------------
Use MCPhysReg instead of uint16_t.


================
Comment at: llvm/lib/Target/CSKY/CSKYISelLowering.cpp:158
+  return Builder.CreateSub(Builder.getIntN(SZ, 1), Call);
+  ;
+}
----------------
Stray semicolon?


================
Comment at: llvm/lib/Target/CSKY/CSKYISelLowering.cpp:241
+      const TargetRegisterClass *RC = nullptr;
+      /*if (VA.needsCustom()) {
+        // f64 are split up into multiple registers or
----------------
Is this commented out code needed?


================
Comment at: llvm/lib/Target/CSKY/CSKYISelLowering.cpp:387
+
+    /*if (VA.needsCustom()) {
+      SDValue Arg = OutVals[i];
----------------
Is this code needed?


================
Comment at: llvm/lib/Target/CSKY/CSKYISelLowering.cpp:489
+
+  /*if (GlobalAddressSDNode *S = dyn_cast<GlobalAddressSDNode>(Callee)) {
+    const GlobalValue *GV = S->getGlobal();
----------------
Is this code needed?


================
Comment at: llvm/lib/Target/CSKY/CSKYInstrFormatsF1.td:1
+//===-- CSKYInstrFormatsF1.td - CSKY Instruction Formats Float1.0-----*- tablegen -*-===//
+//
----------------
80 columns


================
Comment at: llvm/lib/Target/CSKY/CSKYInstrInfoF1.td:1
+//===- CSKYInstrF1.td - CSKY Instruction Float1.0  -*- tablegen -*-=//
+//
----------------
Pad to 80 columns


================
Comment at: llvm/lib/Target/CSKY/CSKYTargetMachine.cpp:101
+
+  std::string CPU = !CPUAttr.hasAttribute(Attribute::None)
+                        ? CPUAttr.getValueAsString().str()
----------------
Replace this with 

```
std::string CPU = CPUAttr.isValid() ? CPUAttr.getValueAsString().str() : TargetCPU;
```

Same for FSAttr. This change made was to all targets in tree a few weeks ago.


================
Comment at: llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCExpr.cpp:58
+
+  // OS << '(';
+  // const MCExpr *Expr = getSubExpr();
----------------
Is this code needed?


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

https://reviews.llvm.org/D86269