Re: [PATCH, i386, MPX 1/X] Support of Intel MPX ISA

[Uros Bizjak <ubizjak at gmail dot com>]

On Tue, Sep 10, 2013 at 1:38 PM, Ilya Enkovich <enkovich.gnu@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.gnu@gmail.com>:
>> Ping^3
>>
>> Attached is the same patch but against the current trunk.
>>
>> 2013/8/26 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>> Ping
>>>
>>> 2013/8/19 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>>> Ping
>>>>
>>>> 2013/8/12 Ilya Enkovich <enkovich.gnu@gmail.com>:
>>>>> 2013/8/10 Joseph S. Myers <joseph@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
>>>>>> joseph@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.enkovich@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.

The x86 part looks mostly OK (I have a couple of comments bellow), but
please first get target-independent changes reviewed and committed.

+         (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.

+{ 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")])

Uros.