This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][Binutils][AArch64] Add verifier for By elem Single and Double sized instructions.


Hi Tamar,

> gas/ChangeLog:
> 2019-02-05  Tamar Christina  <tamar.christina@arm.com>
> 
> 	PR binutils/23212
> 	* testsuite/gas/aarch64/undefined_by_elem_sz_l.s: New test.
> 	* testsuite/gas/aarch64/undefined_by_elem_sz_l.d: New test.
> 
> opcodes/ChangeLog:
> 2019-02-05  Tamar Christina  <tamar.christina@arm.com>
> 
> 	PR binutils/23212
> 	* aarch64-opc.h (enum aarch64_field_kind): Add FLD_sz.
> 	* aarch64-opc.c (verify_elem_sd): New.
> 	(fields): Add FLD_sz entr.
> 	* aarch64-tbl.h (_SIMD_INSN): New.
> 	(aarch64_opcode_table): Add elem_sd verifier to fmla, fmls, fmul and
> 	fmulx scalar and vector by element isns.

Could you make a few changes please:

  +/* Verifier for vector by element 3 operands functions where the
  +   conditions `if sz:L == 11 then UNDEFINED` holds.
  +*/

Minor formatting nit.  The comment closing sequence should be on the 
same line as the end of the comment.  Ie:
 
  +/* Verifier for vector by element 3 operands functions where the
  +   conditions `if sz:L == 11 then UNDEFINED` holds.  */

Also:

  +{
  +  assert (inst->opcode);
  +  assert (inst->opcode->operands[2] == AARCH64_OPND_Em);
  +  const aarch64_insn undef_pattern = 0b11;
  +  aarch64_insn value = encoding ? inst->value : insn;

You have code before variable declarations.  (Which we do have in other
areas, but I am trying to avoid unless it is really helpful for code
clarity).

Plus (and this is the important one) I think that the binary constant 
prefix (0b) is a GCC-ism rather than something that is supported by ISO-C.

Cheers
  Nick


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]