[PATCH] [RFC]TILE-Gx: updated patch for final review

[Sean Silva]

  Some simple stylistic comments. Far from exhaustive.


================
Comment at: lib/Target/Tile/TileFrameLowering.cpp:180-181
@@ +179,4 @@
+    // register to the stack.
+    for (unsigned i = 0; i < CSI.size(); ++i)
+      ++MBBI;
+
----------------
Use std::advance here and in other places

================
Comment at: lib/Target/Tile/TileFrameLowering.cpp:111-112
@@ +110,4 @@
+void TileFrameLowering::emitPrologue(MachineFunction &MF) const {
+  MachineBasicBlock &MBB   = MF.front();
+  MachineFrameInfo *MFI    = MF.getFrameInfo();
+  const TileInstrInfo &TII =
----------------
LLVM style does not align the `=` signs.

================
Comment at: lib/Target/Tile/TileCallingConv.h:32-34
@@ +31,5 @@
+                 CCState &State) {
+
+  if (LocVT == MVT::i32
+      || LocVT == MVT::f32) {
+    unsigned Offset4 = State.AllocateStack(4, 8) + 16;
----------------
this fits on one line (same for the `if` just below this)

================
Comment at: lib/Target/Tile/TileAsmPrinter.cpp:178
@@ +177,3 @@
+    for (unsigned Index = 0;
+         Index < (sizeof bundle_templates / sizeof bundle_templates[0]);
+         Index++) {
----------------
Use array_lengthof

================
Comment at: lib/Target/Tile/MCTargetDesc/TileMCCodeEmitter.cpp:221-223
@@ +220,5 @@
+  if (MO.isReg()) {
+    unsigned Reg = MO.getReg();
+    unsigned RegNo = getTileRegisterNumbering(Reg);
+    return RegNo;
+  } else if (MO.isImm()) {
----------------
this can be just `return getTileRegisterNumbering(MO.getReg());`

================
Comment at: lib/Target/Tile/MCTargetDesc/TileMCCodeEmitter.cpp:170-171
@@ +169,4 @@
+  } else {
+    if (InstBundleBit & Binary)
+      llvm_unreachable("inst bundle bit overlapped!");
+
----------------
Just use an assert(). Here and in other places.

================
Comment at: lib/Target/Tile/AsmParser/TileAsmParser.cpp:395-396
@@ +394,4 @@
+bool TileAsmParser::
+tryParseRelocOperand(SmallVectorImpl<MCParsedAsmOperand*> &Operands,
+                     StringRef Mnemonic){
+
----------------
space before the curly braces.

================
Comment at: lib/Target/Tile/AsmParser/TileAsmParser.cpp:372-383
@@ +371,14 @@
+
+  if (Tok.is(AsmToken::Identifier)) {
+    std::string lowerCase = Tok.getString().lower();
+    if (lowerCase[0] == 'r') {
+      int Value;
+      StringRef Num(lowerCase.c_str() + 1);
+      if (!Num.getAsInteger(10, Value))
+        RegNum = matchRegisterByNumber(Value, Mnemonic.lower());
+    } else
+      RegNum = matchRegisterName(lowerCase, Mnemonic.lower());
+  } 
+
+  return RegNum;
+}
----------------
use an early exit here to simplify. <http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code>

================
Comment at: lib/Target/Tile/AsmParser/TileAsmParser.cpp:350-356
@@ +349,9 @@
+
+unsigned TileAsmParser::
+getReg(int RC,int RegNo){
+  return *(getContext().getRegisterInfo().getRegClass(RC).begin() + RegNo);
+}
+
+int TileAsmParser::
+matchRegisterByNumber(unsigned RegNum,StringRef Mnemonic) {
+
----------------
Put a space after commas, here and elsewhere. Also, try to avoid putting `retty ClassName::` on a separate line. Prefer breaking in the argument list. Alternatively, you could just use clang-format.

================
Comment at: lib/Target/Tile/AsmParser/TileAsmParser.cpp:333-342
@@ +332,12 @@
+  case Match_InvalidOperand: {
+    SMLoc ErrorLoc = IDLoc;
+    if (ErrorInfo != ~0U) {
+      if (ErrorInfo >= Operands.size())
+        return Error(IDLoc, "too few operands for instruction");
+
+      ErrorLoc = ((TileOperand*)Operands[ErrorInfo])->getStartLoc();
+      if (ErrorLoc == SMLoc()) ErrorLoc = IDLoc;
+    }
+
+    return Error(ErrorLoc, "invalid operand for instruction");
+  }
----------------
use early exits to simplify this

================
Comment at: lib/Target/Tile/AsmParser/TileAsmParser.cpp:292-298
@@ +291,9 @@
+
+  if (CC != -1) {
+    if (require32bit(Mnemonic))
+      ++CC;
+    return CC;
+  }
+
+  return -1;
+}
----------------
use an early exit to simplify.

================
Comment at: lib/CodeGen/TargetInfo.cpp:5013-5014
@@ +5012,4 @@
+
+  if (RetTy->isVoidType()
+      || Size == 0)
+    return ABIArgInfo::getIgnore();
----------------
no need for a line break here.

================
Comment at: lib/Target/Tile/TileVLIWPacketizer.cpp:330
@@ +329,3 @@
+  TileSlotType T1 = TileSlotTypeOf(MI1);
+  unsigned count = 0;
+  if (T == SlotX0X1Y0Y1)
----------------
Please follow LLVM naming conventions <http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly>

================
Comment at: lib/Target/Tile/TileVLIWPacketizer.cpp:231
@@ +230,3 @@
+
+  if (SUJ->isSucc(SUI)) {
+    for(unsigned i = 0;
----------------
use an early exit to simplify

================
Comment at: lib/Target/Tile/TileVLIWPacketizer.cpp:259-260
@@ +258,4 @@
+        // do nothing
+      }
+      else if (MCIDI.isConditionalBranch() && (DepType != SDep::Data) &&
+              (DepType != SDep::Output)) {
----------------
please follow our brace style

================
Comment at: lib/Target/Tile/TileVLIWPacketizer.cpp:232-234
@@ +231,5 @@
+  if (SUJ->isSucc(SUI)) {
+    for(unsigned i = 0;
+        (i < SUJ->Succs.size()) && !FoundSequentialDependence;
+        ++i) {
+
----------------
don't evaluate .size() every time through the loop; here and elsewhere.


http://llvm-reviews.chandlerc.com/D573