Re: V4 [PATCH 2/4] i386: Align branches within a fixed boundary

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Thu, Dec 12, 2019 at 1:55 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 11.12.2019 17:58, H.J. Lu wrote:
> > On Wed, Dec 11, 2019 at 12:05 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 10.12.2019 17:21,  H.J. Lu  wrote:
> >>> @@ -8473,9 +8815,105 @@ output_insn (void)
> >>>         if (j > 15)
> >>>           as_warn (_("instruction length of %u bytes exceeds the limit of 15"),
> >>>                    j);
> >>> +       else if (fragP)
> >>> +         {
> >>> +           /* NB: Don't add prefix with GOTPC relocation since
> >>> +              output_disp() above depends on the fixed encoding
> >>> +              length.  Can't add prefix with TLS relocation since
> >>> +              it breaks TLS linker optimization.  */
> >>> +           unsigned int max = i.has_gotpc_tls_reloc ? 0 : 15 - j;
> >>> +           /* Prefix count on the current instruction.  */
> >>> +           unsigned int count = i.vex.length;
> >>> +           unsigned int k;
> >>> +           for (k = 0; k < ARRAY_SIZE (i.prefix); k++)
> >>> +             /* REX byte is encoded in VEX/EVEX prefix.  */
> >>> +             if (i.prefix[k] && (k != REX_PREFIX || !i.vex.length))
> >>> +               count++;
> >>> +
> >>> +           /* Count prefixes for extended opcode maps.  */
> >>> +           if (!i.vex.length)
> >>> +             switch (i.tm.opcode_length)
> >>> +               {
> >>> +               case 3:
> >>> +                 if (((i.tm.base_opcode >> 16) & 0xff) == 0xf)
> >>> +                   {
> >>> +                     count++;
> >>> +                     switch ((i.tm.base_opcode >> 8) & 0xff)
> >>> +                       {
> >>> +                       case 0x38:
> >>> +                       case 0x3a:
> >>> +                         count++;
> >>> +                         break;
> >>> +                       default:
> >>> +                         break;
> >>> +                       }
> >>> +                   }
> >>> +                 break;
> >>> +               case 2:
> >>> +                 if (((i.tm.base_opcode >> 8) & 0xff) == 0xf)
> >>> +                   count++;
> >>> +                 break;
> >>> +               case 1:
> >>> +                 break;
> >>> +               default:
> >>> +                 abort ();
> >>> +               }
> >>> +
> >>> +           if (TYPE_FROM_RELAX_STATE (fragP->fr_subtype)
> >>> +               == BRANCH_PREFIX)
> >>> +             {
> >>> +               /* Set the maximum prefix size in BRANCH_PREFIX
> >>> +                  frag.  */
> >>> +               if (fragP->tc_frag_data.max_bytes > max)
> >>> +                 fragP->tc_frag_data.max_bytes = max;
> >>> +               if (fragP->tc_frag_data.max_bytes > count)
> >>> +                 fragP->tc_frag_data.max_bytes -= count;
> >>> +               else
> >>> +                 fragP->tc_frag_data.max_bytes = 0;
> >>> +             }
> >>> +           else
> >>> +             {
> >>> +               /* Remember the maximum prefix size in FUSED_JCC_PADDING
> >>> +                  frag.  */
> >>> +               unsigned int max_prefix_size;
> >>> +               if (align_branch_prefix_size > max)
> >>> +                 max_prefix_size = max;
> >>> +               else
> >>> +                 max_prefix_size = align_branch_prefix_size;
> >>> +               if (max_prefix_size > count)
> >>> +                 fragP->tc_frag_data.max_prefix_length
> >>> +                   = max_prefix_size - count;
> >>> +             }
> >>> +
> >>> +           /* Use existing segment prefix if possible.  Use CS
> >>> +              segment prefix in 64-bit mode.  In 32-bit mode, use SS
> >>> +              segment prefix with ESP/EBP base register and use DS
> >>> +              segment prefix without ESP/EBP base register.  */
> >>> +           if (i.prefix[SEG_PREFIX])
> >>> +             fragP->tc_frag_data.default_prefix = i.prefix[SEG_PREFIX];
> >>> +           else if (flag_code == CODE_64BIT)
> >>> +             fragP->tc_frag_data.default_prefix = CS_PREFIX_OPCODE;
> >>> +           else if (i.base_reg
> >>> +                    && (i.base_reg->reg_num == 4
> >>> +                        || i.base_reg->reg_num == 5))
> >>> +             fragP->tc_frag_data.default_prefix = SS_PREFIX_OPCODE;
> >>> +           else
> >>> +             fragP->tc_frag_data.default_prefix = DS_PREFIX_OPCODE;
> >>> +         }
> >>
> >> Could you point me at where it is being excluded that any of
> >> these prefixes would get added to insns where they would have
> >> the meaning of a hint (taken / not taken or no-track)?
> >
> > taken / not taken or no-track prefixes are for branches.  We never
> > add prefixes to branches.
>
> But this was my question - could you point me at the code making
> sure this won't happen? I in particular can't spot anything to this
> effect in add_branch_prefix_frag_p().
>

There are

      if (branch)
        /* Skip if this is a branch.  */
        ;
      else if (add_fused_jcc_padding_frag_p ())
...
       else if (add_branch_prefix_frag_p ())

add_branch_prefix_frag_p is never called on branch nor fused jcc.


--
H.J.