2013/9/11 Uros Bizjak <ubiz...@gmail.com>:
> On Tue, Sep 10, 2013 at 1:38 PM, Ilya Enkovich <enkovich....@gmail.com> wrote:
>> Ping^4
>>
>> Could please someone look at this patch? It is mostly i386 target
>> specific and is basic for further MPX based features.
>>
>> Thanks,
>> Ilya
>>
>> 2013/9/2 Ilya Enkovich <enkovich....@gmail.com>:
>>> Ping^3
>>>
>>> Attached is the same patch but against the current trunk.
>>>
>>> 2013/8/26 Ilya Enkovich <enkovich....@gmail.com>:
>>>> Ping
>>>>
>>>> 2013/8/19 Ilya Enkovich <enkovich....@gmail.com>:
>>>>> Ping
>>>>>
>>>>> 2013/8/12 Ilya Enkovich <enkovich....@gmail.com>:
>>>>>> 2013/8/10 Joseph S. Myers <jos...@codesourcery.com>:
>>>>>>> On Mon, 29 Jul 2013, Ilya Enkovich wrote:
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Here is updated version of the patch. I removed redundant
>>>>>>>> mode_for_bound, added comments to BOUND_TYPE and added -mmpx option.
>>>>>>>> I also fixed bndmk/bndldx/bndstx constraints to avoid incorrect
>>>>>>>> register allocation (created two new constraints for that).
>>>>>>>
>>>>>>> I think the -mmpx option should be documented in invoke.texi, and the
>>>>>>> new
>>>>>>> machine modes / mode class should be documented in rtl.texi where other
>>>>>>> machine modes / mode classes are documented. Beyond that, I have no
>>>>>>> comments on this patch revision.
>>>>>>>
>>>>>>> --
>>>>>>> Joseph S. Myers
>>>>>>> jos...@codesourcery.com
>>>>>>
>>>>>> Thanks! Here is a new revision with -mmpx and new machine modes /
>>>>>> class documented.
>>>>>> Is it good to install to trunk?
>>>>>>
>>>>>> Thanks,
>>>>>> Ilya
>>>>>> ---
>>>>>> 2013-08-12 Ilya Enkovich <ilya.enkov...@intel.com>
>>>>>>
>>>>>> * mode-classes.def (MODE_BOUND): New.
>>>>>> * tree.def (BOUND_TYPE): New.
>>>>>> * genmodes.c (complete_mode): Support MODE_BOUND.
>>>>>> (BOUND_MODE): New.
>>>>>> (make_bound_mode): New.
>>>>>> * machmode.h (BOUND_MODE_P): New.
>>>>>> * stor-layout.c (int_mode_for_mode): Support MODE_BOUND.
>>>>>> (layout_type): Support BOUND_TYPE.
>>>>>> * tree-pretty-print.c (dump_generic_node): Support BOUND_TYPE.
>>>>>> * tree.c (build_int_cst_wide): Support BOUND_TYPE.
>>>>>> (type_contains_placeholder_1): Likewise.
>>>>>> * tree.h (BOUND_TYPE_P): New.
>>>>>> * varasm.c (output_constant): Support BOUND_TYPE.
>>>>>> * config/i386/constraints.md (B): New.
>>>>>> (Ti): New.
>>>>>> (Tb): New.
>>>>>> * config/i386/i386-modes.def (BND32): New.
>>>>>> (BND64): New.
>>>>>> * config/i386/i386-protos.h (ix86_bnd_prefixed_insn_p): New.
>>>>>> * config/i386/i386.c (isa_opts): Add mmpx.
>>>>>> (regclass_map): Add bound registers.
>>>>>> (dbx_register_map): Likewise.
>>>>>> (dbx64_register_map): Likewise.
>>>>>> (svr4_dbx_register_map): Likewise.
>>>>>> (PTA_MPX): New.
>>>>>> (ix86_option_override_internal) Support MPX ISA.
>>>>>> (ix86_code_end): Add MPX bnd prefix.
>>>>>> (output_set_got): Likewise.
>>>>>> (ix86_output_call_insn): Likewise.
>>>>>> (get_some_local_dynamic_name): Add '!' (MPX bnd) print prefix
>>>>>> support.
>>>>>> (ix86_print_operand_punct_valid_p): Likewise.
>>>>>> (ix86_print_operand_address): Support UNSPEC_BNDMK_ADDR and
>>>>>> UNSPEC_BNDMK_ADDR.
>>>>>> (ix86_class_likely_spilled_p): Add bound regs support.
>>>>>> (ix86_hard_regno_mode_ok): Likewise.
>>>>>> (x86_order_regs_for_local_alloc): Likewise.
>>>>>> (ix86_bnd_prefixed_insn_p): New.
>>>>>> * config/i386/i386.h (FIRST_PSEUDO_REGISTER): Fix to new value.
>>>>>> (FIXED_REGISTERS): Add bound registers.
>>>>>> (CALL_USED_REGISTERS): Likewise.
>>>>>> (REG_ALLOC_ORDER): Likewise.
>>>>>> (HARD_REGNO_NREGS): Likewise.
>>>>>> (TARGET_MPX): New.
>>>>>> (VALID_BND_REG_MODE): New.
>>>>>> (FIRST_BND_REG): New.
>>>>>> (LAST_BND_REG): New.
>>>>>> (reg_class): Add BND_REGS.
>>>>>> (REG_CLASS_NAMES): Likewise.
>>>>>> (REG_CLASS_CONTENTS): Likewise.
>>>>>> (BND_REGNO_P): New.
>>>>>> (ANY_BND_REG_P): New.
>>>>>> (BNDmode): New.
>>>>>> (HI_REGISTER_NAMES): Add bound registers.
>>>>>> * config/i386/i386.md (UNSPEC_BNDMK): New.
>>>>>> (UNSPEC_BNDMK_ADDR): New.
>>>>>> (UNSPEC_BNDSTX): New.
>>>>>> (UNSPEC_BNDLDX): New.
>>>>>> (UNSPEC_BNDLDX_ADDR): New.
>>>>>> (UNSPEC_BNDCL): New.
>>>>>> (UNSPEC_BNDCU): New.
>>>>>> (UNSPEC_BNDCN): New.
>>>>>> (UNSPEC_MPX_FENCE): New.
>>>>>> (BND0_REG): New.
>>>>>> (BND1_REG): New.
>>>>>> (type): Add mpxmov, mpxmk, mpxchk, mpxld, mpxst.
>>>>>> (length_immediate): Likewise.
>>>>>> (prefix_0f): Likewise.
>>>>>> (memory): Likewise.
>>>>>> (prefix_rep): Check for bnd prefix.
>>>>>> (BND): New.
>>>>>> (bnd_ptr): New.
>>>>>> (BNDCHECK): New.
>>>>>> (bndcheck): New.
>>>>>> (*jcc_1): Add MPX bnd prefix and fix length.
>>>>>> (*jcc_2): Likewise.
>>>>>> (jump): Likewise.
>>>>>> (simple_return_internal): Likewise.
>>>>>> (simple_return_pop_internal): Likewise.
>>>>>> (*indirect_jump): Add MPX bnd prefix.
>>>>>> (*tablejump_1): Likewise.
>>>>>> (simple_return_internal_long): Likewise.
>>>>>> (simple_return_indirect_internal): Likewise.
>>>>>> (<mode>_mk): New.
>>>>>> (*<mode>_mk): New.
>>>>>> (mov<mode>): New.
>>>>>> (*mov<mode>_internal_mpx): New.
>>>>>> (<mode>_<bndcheck>): New.
>>>>>> (*<mode>_<bndcheck>): New.
>>>>>> (<mode>_ldx): New.
>>>>>> (*<mode>_ldx): New.
>>>>>> (<mode>_stx): New.
>>>>>> (*<mode>_stx): New.
>>>>>> * config/i386/predicates.md (lea_address_operand): Rename to...
>>>>>> (address_no_seg_operand): ... this.
>>>>>> (address_mpx_no_base_operand): New.
>>>>>> (address_mpx_no_index_operand): New.
>>>>>> (bnd_mem_operator): New.
>>>>>> * config/i386/i386.opt (mmpx): New.
>>>>>> * doc/invoke.texi: Add documentation for the flags -mmpx,
>>>>>> -mno-mpx.
>>>>>> * doc/rtl.texi (BND32mode): New.
>>>>>> (BND64mode): New.
>>>>>> (MODE_BOUND): New.
>
Hi Uros,
Thanks a lot for the review!
> The x86 part looks mostly OK (I have a couple of comments bellow), but
> please first get target-independent changes reviewed and committed.
Do you mean I should move bound type and mode declaration into a separate patch?
>
> + (plus (const_int 2) (attr "prefix_rep"))
> + (plus (const_int 6) (attr "prefix_rep"))))])
>
> ...
>
> + [(set (attr "length")
> + (plus (const_int 1)
> + (attr "prefix_rep")))
> + (set (attr "prefix_rep")
> + (if_then_else (match_test "ix86_bnd_prefixed_insn_p (insn)")
> + (const_int 1)
> + (const_int 0)))
>
> Generic part already includes adding 1 for "prefix_rep", so these
> additional adds look redundant to me. Also, why you have to calculate
> "prefix_rep" dynamically from ix86_prefixed_insn_p ? Attributes
> correspond to the particular insn pattern, so they can be set
> statically.
Some instructions do not use generic length attribute definition but
still may have rep prefix in MPX code. Thus I have to fix their length
computation. I also have to add prefix_rep computation for ret because
it does not have any type which can be used in generic prefix_rep
computation. ix86_bnd_prefixed_insn_p is used because later (in
subsequent MPX patches) it will decide dynamically which instructions
have prefix and which do not have prefix. E.g. call to MPX function
has prefix but call to legacy (non-MPX) function does not have prefix.
Also it is allowed to have both MPX and non-MPX functions in a single
compilation unit.
>
> +{ 0x1ff100ff,0x1fffe0 }, /* INT_SSE_REGS */ \
> +{ 0x1ff1ffff,0x1fffe0 }, /* FLOAT_INT_SSE_REGS */ \
> +{ 0x0,0x1e00000 }, /* BND_REGS */ \
> +{ 0xffffffff,0x1ffffff } \
>
> Please align numbers to LSB here, as was the case before the change.
>
> +(define_expand "<mode>_mk"
> + [(set (match_operand:BND 0 "register_operand" "=B")
> + (unspec:BND
> + [(mem:<bnd_ptr>
> + (match_par_dup 3
> + [(match_operand:<bnd_ptr> 1 "register_operand" "r")
> + (match_operand:<bnd_ptr> 2 "address_mpx_no_base_operand" "Tb")]))]
> + UNSPEC_BNDMK))]
> + "TARGET_MPX"
>
> Constraints are ignored in expanders, please remove them.
>
> +(define_expand "mov<mode>"
> + [(set (match_operand:BND 0 "general_operand" "=B,m,B")
> + (match_operand:BND 1 "general_operand" "m,B,B"))]
>
> Also here.
>
> +(define_insn "*mov<mode>_internal_mpx"
> + [(set (match_operand:BND 0 "nonimmediate_operand" "=B,m,B")
> + (match_operand:BND 1 "general_operand" "m,B,B"))]
>
> The constraints can be merged to "=B,m" and "Bm,B".
>
> +{
> + return "bndmk\t{%3, %0|%0, %3}";
> +}
>
> Do not use curly braces and return for asm template. The string is enough.
>
> One further question:
>
> +(define_expand "<mode>_mk"
> + [(set (match_operand:BND 0 "register_operand" "=B")
> + (unspec:BND
> + [(mem:<bnd_ptr>
> + (match_par_dup 3
> + [(match_operand:<bnd_ptr> 1 "register_operand" "r")
> + (match_operand:<bnd_ptr> 2 "address_mpx_no_base_operand" "Tb")]))]
> + UNSPEC_BNDMK))]
> + "TARGET_MPX"
> +{
> + operands[3] = gen_rtx_UNSPEC (Pmode, gen_rtvec (2, operands[1],
> + operands[2]),
> + UNSPEC_BNDMK_ADDR);
> +})
>
> Did you check the above with x32, where Pmode != word_mode on x86_64?
> The inner UNSPEC will be generated in SImode, but the matching pattern
>
> +(define_insn "*<mode>_mk"
> + [(set (match_operand:BND 0 "register_operand" "=B")
> + (unspec:BND
> + [(match_operator:<bnd_ptr> 3 "bnd_mem_operator"
> + [(unspec:<bnd_ptr>
> + [(match_operand:<bnd_ptr> 1 "register_operand" "r")
> + (match_operand:<bnd_ptr> 2 "address_mpx_no_base_operand" "Tb")]
> + UNSPEC_BNDMK_ADDR)])]
> + UNSPEC_BNDMK))]
> + "TARGET_MPX"
>
> will have inner UNSPEC in DImode, due to:
>
> +;; Bound modes.
> +(define_mode_iterator BND [(BND32 "!TARGET_64BIT") (BND64 "TARGET_64BIT")])
> +
> +;; Pointer mode corresponding to bound mode.
> +(define_mode_attr bnd_ptr [(BND32 "SI") (BND64 "DI")])
Currently we do not support MPX instrumentation for x32 and therefore
I did not check these expands work correctly for x32. I believe the
only possible problem here is BND iterator definition which does not
care about x32. Following declaration should make everything work
fine:
(define_mode_iterator BND [(BND32 "!TARGET_LP64")
(BND64 "TARGET_LP64")])
BNDmode should also be changed to:
#define BNDmode (TARGET_LP64 ? BND64mode : BND32mode)
Thanks,
Ilya
>
> Uros.
