[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

[Sandeep Patel]

On Tue, Oct 13, 2009 at 8:09 PM, Bob Wilson <bob.wilson at apple.com> wrote:
> 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.

Fixed.

>> +// 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.

I cribbed these helpers from the PPC target.

deep