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

[Jan Beulich]

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?

Jan