[PATCH v2] x86: Add -munaligned-vector-move to assembler

[Michael Matz]

Hello,

On Fri, 22 Oct 2021, H.J. Lu via Binutils wrote:

> On Fri, Oct 22, 2021 at 1:06 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 21.10.2021 19:50, H.J. Lu wrote:
> > > Unaligned load/store instructions on aligned memory or register are as
> > > fast as aligned load/store instructions on modern Intel processors.  Add
> > > a command-line option, -munaligned-vector-move, to x86 assembler to
> > > encode encode aligned vector load/store instructions as unaligned
> > > vector load/store instructions.
> >
> > As said I'm still lacking the spelling out here of some form of
> > motivation for the change. The resulting code, afaict, isn't going
> > to be better than what we have now, yet there's the price of extra
> > new code that you introduce.
> 
> We'd like to have such an option just in case.

As long as it's never going to be default...

I would be quite opposed to making it default.  Linker and assembler 
relaxation are one thing (i.e. the rewriting of certain instruction 
combinations with others), but silently changing an explicitely written 
opcode mnemonic, that doesn't have other forms documented (or where it's 
existing custom that multiple alternatives exist for the assembler to 
choose from), into some other opcode entirely, no matter if, or if they 
currently don't have same semantics for the situation at hand, is 
something else entirely.

Imagine the aligned variants will have slightly different behaviour again 
in the future (or for other processors).  The authors of assembly snippets 
claiming it's a bug that they've requested the aligned variant and the 
assembler had silently given them the unaligned variant would be correct; 
it would be a bug in the assembler.

This is a change that should be done in the compiler.  If you want an 
instruction that can expand to aligned or unaligned variants then you 
could also create a new mnemonic (and document it to be expanding to 
either variant).

Of course, this all being optional on a flag: yeah, well, maybe 
acceptable.  But IMHO even that feels wrong.


Ciao,
Michael.

> 
> > > @@ -4080,6 +4083,26 @@ optimize_encoding (void)
> > >  {
> > >    unsigned int j;
> > >
> > > +  /* Encode aligned vector move as unaligned vector move if asked.  */
> > > +  if (unaligned_vector_move)
> > > +    switch (i.tm.base_opcode)
> > > +      {
> > > +      case 0x28:
> > > +     /* movaps/movapd/vmovaps/vmovapd.  */
> > > +     if (i.tm.opcode_modifier.opcodespace == 1
> > > +         && i.tm.opcode_modifier.opcodeprefix <= 1)
> >
> > I don't think the prefix needs checking here? F3 and F2 encodings
> > don't exist, so maybe at most gas_assert() this?
> 
> I changed it to
> 
>     if (i.tm.opcode_modifier.opcodespace == SPACE_0F
>           && i.tm.opcode_modifier.opcodeprefix <= PREFIX_0X66)
>         i.tm.base_opcode = 0x10;
> 
> in case that F3 and F2 are used in the future.
> 
> > > +       i.tm.base_opcode = 0x10;
> > > +     break;
> > > +      case 0x6f:
> > > +     /* movdqa/vmovdqa/vmovdqa64/vmovdqa32 */
> > > +     if (i.tm.opcode_modifier.opcodespace == 1
> > > +         && i.tm.opcode_modifier.opcodeprefix == 1)
> > > +       i.tm.opcode_modifier.opcodeprefix = 2;
> >
> > Please can you avoid using literal numbers here? This not only makes it
> > needlessly harder to potentially change the SPACE_* and PREFIX_* values
> > (despite me hoping / assuming that we would never have to do so), but
> > also makes the code not sufficiently self-documenting.
> 
> I changed it to
> 
>      if (i.tm.opcode_modifier.opcodespace == SPACE_0F
>           && i.tm.opcode_modifier.opcodeprefix == PREFIX_0X66)
>         i.tm.opcode_modifier.opcodeprefix = PREFIX_0XF3;
> 
> > > @@ -5053,7 +5076,8 @@ md_assemble (char *line)
> > >        i.disp_operands = 0;
> > >      }
> > >
> > > -  if (optimize && !i.no_optimize && i.tm.opcode_modifier.optimize)
> > > +  if (unaligned_vector_move
> > > +      || (optimize && !i.no_optimize && i.tm.opcode_modifier.optimize))
> > >      optimize_encoding ();
> >
> > Isn't this fragile? You now depend on optimize_encoding() to do nothing
> > optimization-wise if unaligned_vector_move is set. I was rather hoping
> > you would introduce a separate helper.
> >
> 
> I added encode_with_unaligned_vector_move and renamed the option
> to -muse-unaligned-vector-move.
> 
> Thanks.
> 
>