[llvm-commits] [llvm] r99322 - /llvm/trunk/lib/Target/ARM/ARMInstrFormats.td

[Bob Wilson]

See comments below:

On Mar 23, 2010, at 1:40 PM, Johnny Chen wrote:

> Author: johnny
> Date: Tue Mar 23 15:40:44 2010
> New Revision: 99322
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=99322&view=rev
> Log:
> Add New NEON Format NVdVmImmFrm.
> 
> Modified:
>    llvm/trunk/lib/Target/ARM/ARMInstrFormats.td
> 
> Modified: llvm/trunk/lib/Target/ARM/ARMInstrFormats.td
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMInstrFormats.td?rev=99322&r1=99321&r2=99322&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMInstrFormats.td (original)
> +++ llvm/trunk/lib/Target/ARM/ARMInstrFormats.td Tue Mar 23 15:40:44 2010
> @@ -59,8 +59,9 @@
> def MiscFrm       : Format<29>;
> def ThumbMiscFrm  : Format<30>;
> 
> -def NLdStFrm      : Format<31>;
> -def NVdImmFrm     : Format<32>;
> +def NLdStFrm                : Format<31>;
> +def NVdImmFrm               : Format<32>;
> +def NVdVmImmFrm             : Format<33>;

I'm not thrilled with these names.  How about "N2RegImmFrm" instead of "NVdVmImmFrm"?  For consistency, "NVdImmFrm" should then be something like "N1RegModImm".

The rest of the patch changes the N2V and N2VX classes to have the new "NVdVmImmFrm" format, but neither of those classes are used for instructions with immediate operands, are they?  You didn't change N2VImm, which _does_ have an immediate operand.

If you intended this new format to describe instructions with 2 registers and NO immediate operand, then it shouldn't have "Imm" in the name of the format.