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

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

On Tue, Dec 10, 2019 at 11:52 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 10.12.2019 18:26, H.J. Lu wrote:
> > On Tue, Dec 10, 2019 at 8:29 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 10.12.2019 17:19,  H.J. Lu  wrote:
> >>> On Mon, Dec 9, 2019 at 6:54 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>> On 06.12.2019 20:10,  H.J. Lu  wrote:
> >>>>> +/* Return 1 if a BRANCH_PREFIX frag should be generated.  */
> >>>>> +
> >>>>> +static int
> >>>>> +add_branch_prefix_frag_p (void)
> >>>>> +{
> >>>>> +  /* NB: Don't work with COND_JUMP86 without i386.  Don't add prefix
> >>>>> +     to PadLock instructions since they include prefixes in opcode.  */
> >>>>> +  if (!align_branch_power
> >>>>> +      || !align_branch_prefix_size
> >>>>> +      || now_seg == absolute_section
> >>>>> +      || i.tm.cpu_flags.bitfield.cpupadlock
> >>>>
> >>>> Didn't you confirm you'd take care of here of other insns than just
> >>>> the PadLock ones including prefixes in their base_opcode? (I still
> >>>
> >>> I added the check for i.tm.opcode_length.   PadLock is the only
> >>> exception.
> >>
> >> I don't see any such check, nor how PadLock would be an exception.
> >> The comment also talks about PadLock only. Please clarify.
> >
> > There are
> >
> >       /* Since the VEX/EVEX prefix contains the implicit prefix, we
> >          don't need the explicit prefix.  */
> >       if (!i.tm.opcode_modifier.vex && !i.tm.opcode_modifier.evex)
> >         {
> >           switch (i.tm.opcode_length)
> >             {
> >             case 3:
> >               if (i.tm.base_opcode & 0xff000000)
> >                 {
> >                   prefix = (i.tm.base_opcode >> 24) & 0xff;
> >                   if (!i.tm.cpu_flags.bitfield.cpupadlock
> >                       || prefix != REPE_PREFIX_OPCODE
> >                       || (i.prefix[REP_PREFIX] != REPE_PREFIX_OPCODE))
> >                     add_prefix (prefix);
> >                 }
> >               break;
> >
> > If it isn't a padlock instruction, a prefix will be added.
>
> But you've noticed the ||-s in there? Certain prefixes will be added
> here even for PadLock insns. Aiui the logic here is to prevent adding
> the same prefix twice, as it appears to be a legal way of coding the
> insns to explicitly prefix them with REPE. IOW I'm still unconvinced
> that your new special case is needed, the more that the logic here is
> entirely for REPE, which you don't mean to add anywhere. Could you
> perhaps give a concrete example of what would go wrong when this
> extra conditional isn't there?
>

There are special checks for i.tm.cpu_flags.bitfield.cpupadlock.  I
want to be conservative not to change these instructions.

-- 
H.J.