[llvm-commits] [llvm] r84009 - in /llvm/trunk: lib/Target/ARM/ARMISelDAGToDAG.cpp lib/Target/ARM/ARMInstrInfo.td lib/Target/ARM/ARMInstrThumb2.td test/CodeGen/ARM/sbfx.ll

[Bob Wilson]

Thanks, Sandeep.  I have a few comments below:

On Oct 13, 2009, at 11:59 AM, Sandeep Patel wrote:
> --- llvm/trunk/lib/Target/ARM/ARMISelDAGToDAG.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMISelDAGToDAG.cpp Tue Oct 13  
> 13:59:48 2009
> @@ -126,6 +126,9 @@
>   /// SelectDYN_ALLOC - Select dynamic alloc for Thumb.
>   SDNode *SelectDYN_ALLOC(SDValue Op);
>
> +  /// SelectV6T2BitfielsOp - Select SBFX/UBFX instructions for ARM.

> +  SDNode *SelectV6T2BitfieldExtractOp(SDValue Op, unsigned Opc);
> +
>

The method name in the comment doesn't match the code.


> +// isInt32Immediate - This method tests to see if a constant operand.
> +// If so Imm will receive the 32 bit value.
> +static bool isInt32Immediate(SDValue N, unsigned &Imm) {
> +  return isInt32Immediate(N.getNode(), Imm);
> +}

This is only called in one place -- wouldn't be better to just add the  
"getNode()" at that one call site?


> +
> +// isOpcWithIntImmediate - This method tests to see if the node is  
> a specific
> +// opcode and that it has a immediate integer right operand.
> +// If so Imm will receive the 32 bit value.
> +static bool isOpcWithIntImmediate(SDNode *N, unsigned Opc,  
> unsigned& Imm) {
> +  return N->getOpcode() == Opc &&
> +         isInt32Immediate(N->getOperand(1).getNode(), Imm);
> +}
> +
> +

Same thing here.  I guess this is just a matter of style, but I would  
just put this in-line in the one place where it is used.  Also, the  
comments for this function and the previous one are not doxygenated.