[llvm-commits] [llvm] r159787 - in /llvm/trunk: lib/Target/Mips/MipsAsmPrinter.cpp test/CodeGen/Mips/inlineasm-operand-code.ll

[Eric Christopher]

On Jul 5, 2012, at 4:58 PM, Jack Carter wrote:

> Author: jacksprat
> Date: Thu Jul  5 18:58:21 2012
> New Revision: 159787
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=159787&view=rev
> Log:
>   Mips specific inline asm operand modifier D.
> 
>   Print the second half of a double word operand.
> 
>   The include list was cleaned up a bit as well.
> 
>   Also the test case was modified to test for both
>   big and little patterns.
> 

Should probably have made this 3 patches.

> 
> +    // This will be shared with other cases in succeeding checkins

You probably meant "successive", but this comment isn't really necessary,
nor is the similar one below. Also, a comment should be a complete prose
sentence:

http://llvm.org/docs/CodingStandards.html#commenting

and the rest of the comments should probably be fixed at some point.

> +    case 'D': {
> +      // Second part of a double word register operand
> +      if (OpNum == 0)
> +        return true;
> +      const MachineOperand &FlagsOP = MI->getOperand(OpNum - 1);
> +      if (!FlagsOP.isImm())
> +        return true;
> +      unsigned Flags = FlagsOP.getImm();
> +      unsigned NumVals = InlineAsm::getNumOperandRegisters(Flags);
> +      if (NumVals != 2) {
> +        if (!Subtarget->isGP32bit() && NumVals == 1 && MO.isReg()) {

Given the comment you probably want isGP64bit()? (Which is what you wrote,
but since you have a predicate I figured the obvious one would be better.

The NumVals comparisons are a bit odd, might want to add a comment for
them.

> +          // In 64 bit mode long longs are always just a single reg

long long is probably a misnomer since we're already in the backend and
such things don't exist.

> +          unsigned Reg = MO.getReg();
> +          O << '$' << MipsInstPrinter::getRegisterName(Reg);
> +          return false;
> +        }
> +        return true;
> +      }
> +      unsigned RegOp;
> +      switch(ExtraCode[0]) {
> +      // This will have other cases in succeeding checkins
> +      case 'D':
> +        RegOp = (!Subtarget->isGP32bit()) ? OpNum : OpNum + 1;
> +        break;
> +      }
> +      if (RegOp >= MI->getNumOperands())
> +        return true;
> +      const MachineOperand &MO = MI->getOperand(RegOp);
> +      if (!MO.isReg())
> +        return true;
> +      unsigned Reg = MO.getReg();
> +      O << '$' << MipsInstPrinter::getRegisterName(Reg);
> +      return false;
>     }
> -  }
> +    } // switch
> +  } // if ExtraCode
> 

Not a huge fan of these comments either :)

-eric

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120705/137ada55/attachment.html>