[llvm-commits] [PATCH][Review request] Mips direct object generation patch #6 of 6

[Bruno Cardoso Lopes]

Hi,

On Tue, Oct 25, 2011 at 10:31 PM, Carter, Jack <jcarter at mips.com> wrote:
> This is the sixth of six patches for Mips direct object generation.
>
> With this final of six patches we can compile and execute a number of simple
> C tests beyond hello.c.
>
>     lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp
>     lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp
>     lib/Target/Mips/MipsInstrInfo.td
>     lib/Target/Mips/Makefile
>
> This patch is a bit meatier than the others, but is more exclusive to the
> Mips direct object production. Once again we tried to pattern after the
> other targets that have already done direct object generation.
>
> Future MIPS direct object generation patches will be fixes and enhancements
> based issues uncovered during testing.
>
> The patch is attached.

A good part of the patch uses 4 spaces instead of 2, please fix that!
Also, don't leave empty newlines here:

+    case Mips::fixup_Mips_LO16:
+    case Mips::fixup_Mips_PC16:
+
+        Value &= 0xffff;
+      break;

Here you don't need braces and don't place the comments above the if condition.

+       if (!Value) {
+           return;           // Doesn't change encoding.
+       }
+       unsigned Offset = Fixup.getOffset();
+       switch (Kind) {
+       default:

Remove the comment below and just leave the llvm_unreachable directly.
If you really want it to "break" here instead, please provide an
explanation of what it does and why in a comment (also don't need to
leave your name in the comment):

+           // I don't know why we get values that are not suppose to be
+           // written out here yet - JCC
+           break;
+         //llvm_unreachable("Unknown fixup kind!");

Proper indent the code below and remove braces:

+     };
+      if (Kind < FirstTargetFixupKind) {
+       return MCAsmBackend::getFixupKindInfo(Kind);
+     }

Ditto again about useless newlines, remove all of them in other places too:

+  if (MO.isReg()) {
+    unsigned Reg = MO.getReg();
+
+    unsigned RegNo = getMipsRegisterNumbering(Reg);
+    return RegNo;
+
+  }

Try to follow more closely llvm style, in the code below, don't use
names like "p_expr" or "e_kind" please:

+  } else if (MO.isExpr()) {
+    const MCExpr *p_expr = MO.getExpr();
+    MCExpr::ExprKind e_kind = p_expr->getKind();
+    if (e_kind == MCExpr::SymbolRef) {

On the snippet below, make this more readable, break it in some local
vars, and then return them.

+    return
+      (getMachineOpValue(MI,
+                         MI.getOperand(OpNo+1),
+                         Fixups) & 0xFFFF) | RegBits;

Also, look at the JIT implementation when implementing the function below:

+/// EncodeInstruction - Emit the instruction.
+/// Size the instruction (currently only 4 bytes and
+/// throwing away any macros.
+void MipsMCCodeEmitter::
+EncodeInstruction(const MCInst &MI, raw_ostream &OS,
+                  SmallVectorImpl<MCFixup> &Fixups) const
+{
+  uint32_t Binary = getBinaryCodeForInstr(MI, Fixups);
+  unsigned opcode = MI.getOpcode();
+
+  const MCInstrDesc &Desc = MCII.get(MI.getOpcode());
+  int Size = Desc.getSize();
+  // If the opcode is a pseudo op its value will be 0 same as a nop
+  if ((!Binary) && (opcode != Mips::SLL) && (opcode != Mips::NOP)) {
+      return;
+  }
+
+  // For now all instructions are 4 bytes
+  Size = 4;
+
+  EmitConstant(Binary, Size, OS);
+}

You must use TSFlags to check for Pseudos, please look at
emitIntruction in MipsCodeEmitter, and do the same thing as possible.

-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc