This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH 3/5] i386: Align branches within a fixed boundary
On Wed, Nov 13, 2019 at 5:15 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 12.11.2019 17:19, H.J. Lu wrote:
> > @@ -632,6 +652,31 @@ static enum check_kind
> > }
> > sse_check, operand_check = check_warning;
> >
> > +/* Non-zero if branches should be aligned within power of 2 boundary. */
> > +static int align_branch_power = 0;
> > +
> > +/* Types of branches to align. */
> > +enum align_branch_kind
> > + {
> > + align_branch_none = 0,
> > + align_branch_jcc = 1 << 0,
> > + align_branch_fused = 1 << 1,
> > + align_branch_jmp = 1 << 2,
> > + align_branch_call = 1 << 3,
> > + align_branch_indirect = 1 << 4,
> > + align_branch_ret = 1 << 5
> > + };
> > +
> > +static unsigned int align_branch = (align_branch_jcc
> > + | align_branch_fused
> > + | align_branch_jmp);
> > +
> > +/* The maximum padding size for fused jcc. */
> > +#define MAX_FUSED_JCC_PADDING_SIZE 20
>
> What is this number derived from?
CMP can be 9 bytes and jcc can be 6 bytes. I leave some room
for prefixes.
> > @@ -3012,6 +3070,11 @@ md_begin (void)
> > x86_dwarf2_return_column = 8;
> > x86_cie_data_alignment = -4;
> > }
> > +
> > + /* NB: FUSED_JCC_PADDING frag must have sufficient room so that it
> > + can be turned into BRANCH_PREFIX frag. */
> > + if (align_branch_prefix_size > MAX_FUSED_JCC_PADDING_SIZE)
> > + abort ();
>
> Wouldn't such a value better be rejected at command line option
> handling time?
There is
/* The maximum number of prefixes added for an instruction. */
static unsigned int align_branch_prefix_size = 5;
I'd like to check it even if it isn't changed.
> > @@ -8178,11 +8252,181 @@ encoding_length (const fragS *start_frag, offsetT start_off,
> > return len - start_off + (frag_now_ptr - frag_now->fr_literal);
> > }
> >
> > +/* Return 1 for test, and, cmp, add, sub, inc and dec which may
> > + be macro-fused with conditional jumps. */
> > +
> > +static int
> > +maybe_fused_with_jcc_p (void)
> > +{
> > + /* No RIP address. */
> > + if (i.base_reg && i.base_reg->reg_num == RegIP)
> > + return 0;
> > +
> > + /* and, add, sub with destination register. */
> > + if (!strcmp (i.tm.name, "and")
> > + || !strcmp (i.tm.name, "add")
> > + || !strcmp (i.tm.name, "sub"))
> > + return i.types[1].bitfield.class == Reg;
> > +
> > + /* test, cmp with any register. */
> > + if (!strcmp (i.tm.name, "test") || !strcmp (i.tm.name, "cmp"))
> > + return (i.types[0].bitfield.class == Reg
> > + || i.types[1].bitfield.class == Reg);
> > +
> > + /* inc, dec with 16/32/64-bit register. */
> > + if (!strcmp (i.tm.name, "inc") || !strcmp (i.tm.name, "dec"))
> > + return i.types[0].bitfield.class == Reg;
>
> Code and comment don't match up: Either you also mean 8-bit
> registers and comment is wrong, or you need to extend the
> condition / return value expression.
Fixed.
> Furthermore with this being executed quite frequently, I
> think using strcmp() isn't a very performant approach here.
I timed a big assembly file:
[hjl@gnu-cfl-1 jcc-11]$ ls -lh torque_base_jumbo_2.s
-rw-rw-r-- 1 hjl hjl 154M Nov 13 08:13 torque_base_jumbo_2.s
[hjl@gnu-cfl-1 jcc-11]$ time ./as -o torque_base_jumbo_2.o
torque_base_jumbo_2.s -mbranches-within-32B-boundaries
real 0m7.534s
user 0m7.204s
sys 0m0.318s
[hjl@gnu-cfl-1 jcc-11]$ time ./as -o torque_base_jumbo_2.o torque_base_jumbo_2.s
real 0m7.472s
user 0m7.152s
sys 0m0.305s
[hjl@gnu-cfl-1 jcc-11]$
Time difference is very small. We can improve it later.
> Like elsewhere I think you want to use i.tm checks here. This
> would also eliminate my worry of the code not catching cases
> where optimization changes the opcode in i.tm without also
> changing the name.
It don't think CMP instructions can be replaced by totally different
instructions.
> > +static int
> > +add_fused_jcc_padding_frag_p (void)
> > +{
> > + if (!align_branch_power
> > + || now_seg == absolute_section
> > + || !cpu_arch_flags.bitfield.cpui386
>
> Why the i386 dependency?
It doesn't work with COND_JUMP86 without i386.
> > + || !(align_branch & align_branch_fused))
> > + return 0;
> > +
> > + if (maybe_fused_with_jcc_p ())
> > + {
> > + if (last_insn.kind != last_insn_other
> > + && last_insn.seg == now_seg)
> > + {
> > + if (flag_debug)
> > + as_warn_where (last_insn.file, last_insn.line,
> > + _("`%s` skips -malign-branch-boundary on `%s`"),
> > + last_insn.name, i.tm.name);
> > + return 0;
> > + }
> > + return 1;
> > + }
> > +
> > + return 0;
>
> Maybe slightly better (less indentation and a folded return) as
>
> if (maybe_fused_with_jcc_p ())
> {
> if (last_insn.kind == last_insn_other
> || last_insn.seg != now_seg)
> return 1;
> if (flag_debug)
> as_warn_where (last_insn.file, last_insn.line,
> _("`%s` skips -malign-branch-boundary on `%s`"),
> last_insn.name, i.tm.name);
> }
>
> return 0;
>
> ?
Fixed.
> > +}
> > +
> > +/* Return 1 if a BRANCH_PREFIX frag should be generated. */
> > +
> > +static int
> > +add_branch_prefix_frag_p (void)
> > +{
> > + if (!align_branch_power
> > + || now_seg == absolute_section
> > + || i.tm.cpu_flags.bitfield.cpupadlock
> > + || !cpu_arch_flags.bitfield.cpui386)
>
> Why the PadLock and i386 dependencies?
Since PadLock instructions include prefixes in opcode, I don't
want to mess with them.
> > + return 0;
> > +
> > + /* Don't add prefix if it is a prefix or there is no operand. */
> > + if (!i.operands || i.tm.opcode_modifier.isprefix)
> > + return 0;
> > +
> > + if (last_insn.kind != last_insn_other
> > + && last_insn.seg == now_seg)
> > + {
> > + if (flag_debug)
> > + as_warn_where (last_insn.file, last_insn.line,
> > + _("`%s` skips -malign-branch-boundary on `%s`"),
> > + last_insn.name, i.tm.name);
> > + return 0;
> > + }
> > +
> > + return 1;
>
> Like above this too can be had with less indentation.
Fixed.
> > +static int
> > +add_branch_padding_frag_p (enum align_branch_kind *branch_p)
> > +{
> > + int add_padding;
> > +
> > + if (!align_branch_power
> > + || now_seg == absolute_section
> > + || !cpu_arch_flags.bitfield.cpui386)
>
> Same question as above.
>
> > + return 0;
> > +
> > + add_padding = 0;
> > +
> > + /* Check for jcc and direct jmp. */
> > + if (i.tm.opcode_modifier.jump)
> > + {
> > + if (i.tm.base_opcode == JUMP_PC_RELATIVE)
> > + {
> > + *branch_p = align_branch_jmp;
> > + add_padding = align_branch & align_branch_jmp;
> > + }
> > + else
> > + {
> > + *branch_p = align_branch_jcc;
> > + if ((align_branch & align_branch_jcc))
> > + add_padding = 1;
> > + }
> > + }
> > + else if (i.tm.base_opcode == 0xc2
> > + || i.tm.base_opcode == 0xc3
> > + || i.tm.base_opcode == 0xca
> > + || i.tm.base_opcode == 0xcb)
>
> These will also match various VEX/XOP/EVEX encoded templates. Perhaps
> exclude any such right away in the first if()?
>
> Also you handle near and far returns here, but ...
I will change it to
else if (is_any_vex_encoding (&i.tm))
return 0;
else if (i.tm.base_opcode == 0xc2 || i.tm.base_opcode == 0xc3)
{
/* Near ret. */
*branch_p = align_branch_ret;
if ((align_branch & align_branch_ret))
add_padding = 1;
}
> > + {
> > + *branch_p = align_branch_ret;
> > + if ((align_branch & align_branch_ret))
> > + add_padding = 1;
> > + }
> > + else
> > + {
> > + if (i.tm.base_opcode == 0xe8)
> > + {
> > + *branch_p = align_branch_call;
> > + if ((align_branch & align_branch_call))
> > + add_padding = 1;
> > + }
> > + else if (i.tm.base_opcode == 0xff
> > + && (i.rm.reg == 2 || i.rm.reg == 4))
>
> ... only near indirect call/jmp here?
Fart ret will be removed.
> It would also seem more consistent to be if you used
> i.tm.extension_opcode instead of i.rm.reg for the checks here.
Fixed.
> > + {
> > + *branch_p = align_branch_indirect;
> > + if ((align_branch & align_branch_indirect))
> > + add_padding = 1;
> > + }
> > +
> > + /* Check for indirect jmp, direct and indirect calls. */
>
> I guess this comment really belongs several lines up from here (and
> a similar one is missing ahead of the RET related conditional)?
Fixed.
> > @@ -8279,6 +8523,30 @@ output_insn (void)
> > insn_start_frag = frag_now;
> > insn_start_off = frag_now_fix ();
> >
> > + if (add_branch_padding_frag_p (&branch))
> > + {
> > + char *p;
> > + unsigned int max_branch_padding_size = 14;
>
> Where is this number coming from? Like above (and I think also in
> one more case below) any such seemingly arbitrary numbers could do
> with a comment.
Branch can be 8 bytes. I leave some room for prefixes.
> > @@ -8317,6 +8585,44 @@ output_insn (void)
> > i.prefix[LOCK_PREFIX] = 0;
> > }
> >
> > + if (branch)
> > + /* Skip if this is a branch. */
> > + ;
> > + else if (add_fused_jcc_padding_frag_p ())
> > + {
> > + unsigned int max_fused_padding_size
> > + = MAX_FUSED_JCC_PADDING_SIZE;
>
> Why the local variable? You could as well use the manifest constant
> at all the use sites below as it seems. If it was just for the
> identifier length, then I guess the variable could do with further
> shortening.
I will remove max_fused_padding_size and use
MAX_FUSED_JCC_PADDING_SIZE.
> > @@ -8461,12 +8767,81 @@ output_insn (void)
> > if (now_seg != absolute_section)
> > {
> > j = encoding_length (insn_start_frag, insn_start_off, frag_more (0));
> > - if (j > 15)
> > + if (fragP)
> > + {
> > + /* NB: Don't add prefix with GOTPC relocation since
> > + output_disp() above depends on the fixed encoding
> > + length. */
> > + unsigned int max = i.has_gotpc_reloc ? 0 : 15 - j;
> > + /* Prefix count on the current instruction. */
> > + unsigned int count = !!is_any_vex_encoding (&i.tm);
> > + unsigned int k;
> > + for (k = 0; k < ARRAY_SIZE (i.prefix); k++)
> > + if (i.prefix[k])
> > + count++;
> > +
> > + /* NB: prefix count + instruction size must be <= 15. */
> > + if (j > 15)
> > + as_fatal (_("instruction length of %u bytes exceeds the limit of 15"),
> > + j);
>
> I think you'd better keep the if (j > 15) as the first check above
> and make the new code an else-if to it. You shouldn't fail assembly
> in this case, whereas the warning implying no padding attempt to
> get done seems quite reasonable a behavior to me.
Fixed.
> > + 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. */
>
> Is there a reason for the preference of CS: in 64-bit mode?
CS is ignored in 64-bit mode.
> > +static void
> > +i386_classify_machine_dependent_frag (fragS *fragP)
> > +{
> > + fragS *cmp_fragP;
> > + fragS *pad_fragP;
> > + fragS *branch_fragP;
> > + fragS *next_fragP;
> > + unsigned int max_prefix_length;
> > +
> > + if (fragP->tc_frag_data.classified)
> > + return;
> > +
> > + /* First scan for BRANCH_PADDING and FUSED_JCC_PADDING. Convert
> > + FUSED_JCC_PADDING and merge BRANCH_PADDING. */
> > + for (next_fragP = fragP;
> > + next_fragP != NULL;
> > + next_fragP = next_fragP->fr_next)
> > + {
> > + next_fragP->tc_frag_data.classified = 1;
> > + if (next_fragP->fr_type == rs_machine_dependent)
> > + switch (TYPE_FROM_RELAX_STATE (next_fragP->fr_subtype))
> > + {
> > + case BRANCH_PADDING:
> > + /* The BRANCH_PADDING frag must be followed by a branch
> > + frag. */
> > + branch_fragP = i386_next_non_empty_frag (next_fragP);
> > + next_fragP->tc_frag_data.u.branch_fragP = branch_fragP;
> > + break;
> > + case FUSED_JCC_PADDING:
> > + /* Check if this is a fused jcc:
> > + FUSED_JCC_PADDING
> > + CMP
>
> Here and in a number of other places I wonder why you say CMP when
> the set of macro-fusion candidate insns is quite a bit larger.
I use CMP to refer all these instructions.
> > + BRANCH_PADDING
> > + COND_JUMP
> > + */
> > + cmp_fragP = i386_next_non_empty_frag (next_fragP);
> > + pad_fragP = i386_next_non_empty_frag (cmp_fragP);
> > + branch_fragP = i386_next_jcc_frag (pad_fragP);
> > + if (branch_fragP)
> > + {
> > + /* The BRANCH_PADDING frag is merged with the
> > + FUSED_JCC_PADDING frag. */
> > + next_fragP->tc_frag_data.u.branch_fragP = branch_fragP;
> > + /* CMP instruction size. */
> > + next_fragP->tc_frag_data.cmp_size = cmp_fragP->fr_fix;
> > + frag_wane (pad_fragP);
> > + /* Skip to branch_fragP. */
> > + next_fragP = branch_fragP;
> > + }
> > + else if (next_fragP->tc_frag_data.max_prefix_length)
> > + {
> > + /* Turn FUSED_JCC_PADDING into BRANCH_PREFIX if it isn't
> > + a fused jcc. */
> > + next_fragP->fr_subtype
> > + = ENCODE_RELAX_STATE (BRANCH_PREFIX, 0);
> > + next_fragP->tc_frag_data.max_bytes
> > + = next_fragP->tc_frag_data.max_prefix_length;
> > + /* This will be updated in the BRANCH_PREFIX scan. */
> > + next_fragP->tc_frag_data.max_prefix_length = 0;
> > + }
> > + else
> > + frag_wane (next_fragP);
> > + break;
> > + }
> > + }
> > +
> > + /* Scan for BRANCH_PREFIX. */
> > + for (; fragP != NULL; fragP = fragP->fr_next)
> > + if (fragP->fr_type == rs_machine_dependent
> > + && (TYPE_FROM_RELAX_STATE (fragP->fr_subtype)
> > + == BRANCH_PREFIX))
> > + {
> > + /* Count all BRANCH_PREFIX frags before BRANCH_PADDING and
> > + COND_JUMP_PREFIX. */
> > + max_prefix_length = 0;
> > + for (next_fragP = fragP;
> > + next_fragP != NULL;
> > + next_fragP = next_fragP->fr_next)
> > + {
> > + if (next_fragP->fr_type == rs_fill)
> > + /* Skip rs_fill frags. */
> > + ;
> > + else if (next_fragP->fr_type == rs_machine_dependent)
> > + {
> > + if (TYPE_FROM_RELAX_STATE (next_fragP->fr_subtype)
> > + == BRANCH_PREFIX)
> > + {
> > + /* Count BRANCH_PREFIX frags. */
> > + if (max_prefix_length >= MAX_FUSED_JCC_PADDING_SIZE)
> > + {
> > + max_prefix_length = MAX_FUSED_JCC_PADDING_SIZE;
> > + frag_wane (next_fragP);
> > + }
> > + else
> > + max_prefix_length
> > + += next_fragP->tc_frag_data.max_bytes;
> > + }
> > + else if ((TYPE_FROM_RELAX_STATE (next_fragP->fr_subtype)
> > + == BRANCH_PADDING)
> > + || (TYPE_FROM_RELAX_STATE (next_fragP->fr_subtype)
> > + == FUSED_JCC_PADDING))
> > + {
> > + /* Stop at BRANCH_PADDING and FUSED_JCC_PADDING. */
> > + fragP->tc_frag_data.u.padding_fragP = next_fragP;
> > + break;
> > + }
> > + else
> > + /* Stop for other rs_machine_dependent frags. */
> > + break;
> > + }
> > + else
> > + /* Stop for all other frags. */
> > + break;
>
> Some indentation depth could again be saved by re-arranging the code.
I will try it.
> > +static int
> > +i386_branch_padding_size (fragS *fragP, offsetT address)
> > +{
> > + unsigned int offset, size, padding_size;
> > + fragS *branch_fragP = fragP->tc_frag_data.u.branch_fragP;
> > +
> > + /* The start address of the BRANCH_PADDING or FUSED_JCC_PADDING frag. */
> > + if (!address)
> > + address = fragP->fr_address;
> > + address += fragP->fr_fix;
> > +
> > + /* CMP instrunction size. */
> > + size = fragP->tc_frag_data.cmp_size;
> > +
> > + /* The base size of the branch frag. */
> > + size += branch_fragP->fr_fix;
> > +
> > + /* Add opcode and displacement bytes for the rs_machine_dependent
> > + branch frag. */
> > + if (branch_fragP->fr_type == rs_machine_dependent)
> > + size += md_relax_table[branch_fragP->fr_subtype].rlx_length;
> > +
> > + /* Check if branch is within boundary and doesn't end at the last
> > + byte. */
> > + offset = address & ((1U << align_branch_power) - 1);
> > + if ((offset + size) >= (1U << align_branch_power))
> > + /* Padding needed to avoid crossing boundary. */
> > + padding_size = (1 << align_branch_power) - offset;
>
> Shouldn't command line option handling make sure the shifts here
> won't become undefined due to too large a shift count? Also isn't
> the last 1 missing a U suffix?
I will add 1U and limit align_branch_power to 31.
> > + else
> > + /* No padding needed. */
> > + padding_size = 0;
> > +
> > + if (!fits_in_signed_byte (padding_size))
> > + abort ();
>
> Why? The function's return type is int (should probably be
> unsigned int).
It must fit tc_frag_data.length.
> > @@ -11483,6 +12323,78 @@ md_parse_option (int c, const char *arg)
> > as_fatal (_("invalid -mrelax-relocations= option: `%s'"), arg);
> > break;
> >
> > + case OPTION_MALIGN_BRANCH_BOUNDARY:
> > + {
> > + char *end;
> > + int align = strtoul (arg, &end, 0);
> > + if (*end == '\0')
> > + {
> > + if (align == 0)
> > + {
> > + align_branch_power = 0;
> > + break;
> > + }
> > + else if (align >= 32)
>
> Why the enforcement of 32 as the lower boundary? I.e. why would 16
> not be an option as well?
CMP can be 9 bytes and jcc can be 6 bytes. 15 bytes is too close to
16 bytes.
> > + {
> > + int align_power;
> > + for (align_power = 0;
> > + (align & 1) == 0;
> > + align >>= 1, align_power++)
> > + continue;
> > + if (align == 1)
> > + {
> > + align_branch_power = align_power;
> > + break;
> > + }
> > + }
> > + }
> > + as_fatal (_("invalid -malign-branch-boundary= value: %s"), arg);
> > + }
> > + break;
> > +
> > + case OPTION_MALIGN_BRANCH_PREFIX_SIZE:
> > + {
> > + char *end;
> > + int align = strtoul (arg, &end, 0);
> > + if (*end == '\0' && align >= 0 && align < 6)
>
> Similarly here - why is 5 the upper boundary on the accepted values?
I am told that some processors only support 5 prefixes.
> > + {
> > + align_branch_prefix_size = align;
> > + break;
> > + }
> > + as_fatal (_("invalid -malign-branch-prefix-size= value: %s"),
> > + arg);
> > + }
> > + break;
> > +
> > + case OPTION_MALIGN_BRANCH:
> > + align_branch = 0;
> > + saved = xstrdup (arg);
> > + type = saved;
> > + do
> > + {
> > + next = strchr (type, '+');
> > + if (next)
> > + *next++ = '\0';
> > + if (strcasecmp (type, "jcc") == 0)
> > + align_branch |= align_branch_jcc;
> > + else if (strcasecmp (type, "fused") == 0)
> > + align_branch |= align_branch_fused;
> > + else if (strcasecmp (type, "jmp") == 0)
> > + align_branch |= align_branch_jmp;
> > + else if (strcasecmp (type, "call") == 0)
> > + align_branch |= align_branch_call;
> > + else if (strcasecmp (type, "ret") == 0)
> > + align_branch |= align_branch_ret;
> > + else if (strcasecmp (type, "indirect") == 0)
> > + align_branch |= align_branch_indirect;
>
> Aren't command line options generally case sensitive, i.e. wouldn't
> you better use strcmp() here?
I'd like to leave strcasecmp there just in case.
> > @@ -11943,6 +12875,21 @@ s_bss (int ignore ATTRIBUTE_UNUSED)
> >
> > #endif
> >
> > +/* Remember constant diretive. */
> > +
> > +void
> > +i386_cons_worker (int ignore ATTRIBUTE_UNUSED)
> > +{
> > + if (last_insn.kind != last_insn_directive
> > + && (bfd_section_flags (now_seg) & SEC_CODE))
> > + {
> > + last_insn.seg = now_seg;
> > + last_insn.kind = last_insn_directive;
> > + last_insn.name = "constant diretive";
>
> Nit: Missing 'c' in directive.
I will fix it.
> > @@ -250,10 +257,24 @@ extern i386_cpu_flags cpu_arch_isa_flags;
> >
> > struct i386_tc_frag_data
> > {
> > + union
> > + {
> > + fragS *padding_fragP;
> > + fragS *branch_fragP;
> > + } u;
> > + addressT padding_address;
> > enum processor_type isa;
> > i386_cpu_flags isa_flags;
> > enum processor_type tune;
> > unsigned int max_bytes;
> > + signed char length;
> > + signed char last_length;
> > + signed char max_prefix_length;
> > + signed char prefix_length;
> > + signed char default_prefix;
> > + signed char cmp_size;
>
> Why signed char for all of these? Most if not all look to be
> holding unsigned quantities only.
I will change it to unsigned char.
> > + unsigned int classified : 1;
> > + unsigned int branch_type : 7;
>
> This doesn't require 7 bits, does it?
>
I will change it to
unsigned int branch_type : 6;
Thanks.
--
H.J.