diff mbox

[i386,Pointer,Bounds,Checker,30/x] Size relocation

Message ID 20140918140033.GB50194@msticlxl57.ims.intel.com
State New
Headers show

Commit Message

Ilya Enkovich Sept. 18, 2014, 2 p.m. UTC 
On 17 Sep 20:51, Uros Bizjak wrote:
> On Wed, Sep 17, 2014 at 8:35 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> > On 16 Sep 12:22, Uros Bizjak wrote:
> >> On Tue, Sep 16, 2014 at 11:37 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> >> > 2014-09-16 13:08 GMT+04:00 Uros Bizjak <ubizjak@gmail.com>:
> >> >>
> >> >> Can x86_64_immediate_operand predicate be used here?
> >> >
> >> > I think it cannot be used because of TLS symbols not counting as immediate.
> >>
> >> OK, please introduce a new predicate, similar to
> >> x86_64_immediate_operand, perhaps x86_64_immediate_size_operand, so we
> >> can add some comments there. This will also help to macroize the insn,
> >> x86_64_immediate_operand has !TARGET_64BIT shortcut for this case.
> >>
> >> Uros.
> >
> > I don't see how new predicate would help to macroize insn.  Single template may look as following patch.
> 
> You put early return for !TARGET_64BITS. Please see
> x86_64_immediate_operand predicate.
> 
> So,
> 
> /* Here comes comment. */
> (define_predicate "x86_64_immediate_size_operand"
>   (match_code "symbol_ref")
> {
>   if (!TARGET_64BIT)
>     return true;
> 
>   /* Comment here explaining these conditions.  */
>   return (ix86_cmodel == CM_SMALL || ix86_cmodel == CM_KERNEL);
> }
> 
> And then in the pattern itself:
> 
> if (x86_64_immediate_size_operand (operands[1], VOIDmode)
>   return "mov{l}\t{%1@SIZE, %k0|%k0, %1@SIZE}";
> else
>   return "movabs{q}\t{%1@SIZE, %0|%0, %1@SIZE}";
> 
> Uros.

Here is a version with check in a form you suggest.

Thanks,
Ilya
--
2014-09-18  Ilya Enkovich  <ilya.enkovich@intel.com>

	* config/i386/i386.md (UNSPEC_SIZEOF): New.
	(move_size_reloc_<mode>): New.
	* config/i386/predicates.md (symbol_operand): New.
	(x86_64_immediate_size_operand): New.

Comments

Uros Bizjak Sept. 18, 2014, 5:27 p.m. UTC | #1 
On Thu, Sep 18, 2014 at 4:00 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> On 17 Sep 20:51, Uros Bizjak wrote:
>> On Wed, Sep 17, 2014 at 8:35 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> > On 16 Sep 12:22, Uros Bizjak wrote:
>> >> On Tue, Sep 16, 2014 at 11:37 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> >> > 2014-09-16 13:08 GMT+04:00 Uros Bizjak <ubizjak@gmail.com>:
>> >> >>
>> >> >> Can x86_64_immediate_operand predicate be used here?
>> >> >
>> >> > I think it cannot be used because of TLS symbols not counting as immediate.
>> >>
>> >> OK, please introduce a new predicate, similar to
>> >> x86_64_immediate_operand, perhaps x86_64_immediate_size_operand, so we
>> >> can add some comments there. This will also help to macroize the insn,
>> >> x86_64_immediate_operand has !TARGET_64BIT shortcut for this case.
>> >>
>> >> Uros.
>> >
>> > I don't see how new predicate would help to macroize insn.  Single template may look as following patch.
>>
>> You put early return for !TARGET_64BITS. Please see
>> x86_64_immediate_operand predicate.
>>
>> So,
>>
>> /* Here comes comment. */
>> (define_predicate "x86_64_immediate_size_operand"
>>   (match_code "symbol_ref")
>> {
>>   if (!TARGET_64BIT)
>>     return true;
>>
>>   /* Comment here explaining these conditions.  */
>>   return (ix86_cmodel == CM_SMALL || ix86_cmodel == CM_KERNEL);
>> }
>>
>> And then in the pattern itself:
>>
>> if (x86_64_immediate_size_operand (operands[1], VOIDmode)
>>   return "mov{l}\t{%1@SIZE, %k0|%k0, %1@SIZE}";
>> else
>>   return "movabs{q}\t{%1@SIZE, %0|%0, %1@SIZE}";
>>
>> Uros.
>
> Here is a version with check in a form you suggest.
>
> Thanks,
> Ilya
> --
> 2014-09-18  Ilya Enkovich  <ilya.enkovich@intel.com>
>
>         * config/i386/i386.md (UNSPEC_SIZEOF): New.
>         (move_size_reloc_<mode>): New.
>         * config/i386/predicates.md (symbol_operand): New.
>         (x86_64_immediate_size_operand): New.

OK.

We are always on the safe side now, movl is an optimization exception.
I wonder if we can also add something like

          || (ix86_cmodel == CM_MEDIUM && !SYMBOL_REF_FAR_ADDR_P (op)));

as is the case with x86_64_immediate_operand, but I am not sure that
object size is guaranteed to fit in 31bits. Maybe Honza (CC'd) can
confirm this.

Uros.

> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index 2c367b2..db22b06 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -79,6 +79,7 @@
>    UNSPEC_PLTOFF
>    UNSPEC_MACHOPIC_OFFSET
>    UNSPEC_PCREL
> +  UNSPEC_SIZEOF
>
>    ;; Prologue support
>    UNSPEC_STACK_ALLOC
> @@ -18554,6 +18555,21 @@
>    "bndstx\t{%2, %3|%3, %2}"
>    [(set_attr "type" "mpxst")])
>
> +(define_insn "move_size_reloc_<mode>"
> +  [(set (match_operand:SWI48 0 "register_operand" "=r")
> +       (unspec:SWI48
> +        [(match_operand:SWI48 1 "symbol_operand")]
> +        UNSPEC_SIZEOF))]
> +  "TARGET_MPX"
> +{
> +  if (x86_64_immediate_size_operand (operands[1], VOIDmode))
> +    return "mov{l}\t{%1@SIZE, %k0|%k0, %1@SIZE}";
> +  else
> +    return "movabs{q}\t{%1@SIZE, %0|%0, %1@SIZE}";
> +}
> +  [(set_attr "type" "imov")
> +   (set_attr "mode" "<MODE>")])
> +
>  (include "mmx.md")
>  (include "sse.md")
>  (include "sync.md")
> diff --git a/gcc/config/i386/predicates.md b/gcc/config/i386/predicates.md
> index cd542b7..da01c9a 100644
> --- a/gcc/config/i386/predicates.md
> +++ b/gcc/config/i386/predicates.md
> @@ -124,6 +124,10 @@
>         (match_test "TARGET_64BIT")
>         (match_test "REGNO (op) > BX_REG")))
>
> +;; Return true if VALUE is symbol reference
> +(define_predicate "symbol_operand"
> +  (match_code "symbol_ref"))
> +
>  ;; Return true if VALUE can be stored in a sign extended immediate field.
>  (define_predicate "x86_64_immediate_operand"
>    (match_code "const_int,symbol_ref,label_ref,const")
> @@ -336,6 +340,19 @@
>    return false;
>  })
>
> +;; Return true if size of VALUE can be stored in a sign
> +;; extended immediate field.
> +(define_predicate "x86_64_immediate_size_operand"
> +  (match_code "symbol_ref")
> +{
> +  if (!TARGET_64BIT)
> +    return true;
> +
> +  /* For 64 bit target we may assume size of object fits
> +     immediate only when code model guarantees that.  */
> +  return (ix86_cmodel == CM_SMALL || ix86_cmodel == CM_KERNEL);
> +})
> +
>  ;; Return true if OP is general operand representable on x86_64.
>  (define_predicate "x86_64_general_operand"
>    (if_then_else (match_test "TARGET_64BIT")
Ilya Enkovich Oct. 1, 2014, 2:10 p.m. UTC | #2 
2014-09-18 18:00 GMT+04:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
> On 17 Sep 20:51, Uros Bizjak wrote:
>> On Wed, Sep 17, 2014 at 8:35 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> > On 16 Sep 12:22, Uros Bizjak wrote:
>> >> On Tue, Sep 16, 2014 at 11:37 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> >> > 2014-09-16 13:08 GMT+04:00 Uros Bizjak <ubizjak@gmail.com>:
>> >> >>
>> >> >> Can x86_64_immediate_operand predicate be used here?
>> >> >
>> >> > I think it cannot be used because of TLS symbols not counting as immediate.
>> >>
>> >> OK, please introduce a new predicate, similar to
>> >> x86_64_immediate_operand, perhaps x86_64_immediate_size_operand, so we
>> >> can add some comments there. This will also help to macroize the insn,
>> >> x86_64_immediate_operand has !TARGET_64BIT shortcut for this case.
>> >>
>> >> Uros.
>> >
>> > I don't see how new predicate would help to macroize insn.  Single template may look as following patch.
>>
>> You put early return for !TARGET_64BITS. Please see
>> x86_64_immediate_operand predicate.
>>
>> So,
>>
>> /* Here comes comment. */
>> (define_predicate "x86_64_immediate_size_operand"
>>   (match_code "symbol_ref")
>> {
>>   if (!TARGET_64BIT)
>>     return true;
>>
>>   /* Comment here explaining these conditions.  */
>>   return (ix86_cmodel == CM_SMALL || ix86_cmodel == CM_KERNEL);
>> }
>>
>> And then in the pattern itself:
>>
>> if (x86_64_immediate_size_operand (operands[1], VOIDmode)
>>   return "mov{l}\t{%1@SIZE, %k0|%k0, %1@SIZE}";
>> else
>>   return "movabs{q}\t{%1@SIZE, %0|%0, %1@SIZE}";
>>
>> Uros.
>
> Here is a version with check in a form you suggest.
>
> Thanks,
> Ilya
> --
> 2014-09-18  Ilya Enkovich  <ilya.enkovich@intel.com>
>
>         * config/i386/i386.md (UNSPEC_SIZEOF): New.
>         (move_size_reloc_<mode>): New.
>         * config/i386/predicates.md (symbol_operand): New.
>         (x86_64_immediate_size_operand): New.
>
>
> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index 2c367b2..db22b06 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -79,6 +79,7 @@
>    UNSPEC_PLTOFF
>    UNSPEC_MACHOPIC_OFFSET
>    UNSPEC_PCREL
> +  UNSPEC_SIZEOF
>
>    ;; Prologue support
>    UNSPEC_STACK_ALLOC
> @@ -18554,6 +18555,21 @@
>    "bndstx\t{%2, %3|%3, %2}"
>    [(set_attr "type" "mpxst")])
>
> +(define_insn "move_size_reloc_<mode>"
> +  [(set (match_operand:SWI48 0 "register_operand" "=r")
> +       (unspec:SWI48
> +        [(match_operand:SWI48 1 "symbol_operand")]
> +        UNSPEC_SIZEOF))]
> +  "TARGET_MPX"
> +{
> +  if (x86_64_immediate_size_operand (operands[1], VOIDmode))
> +    return "mov{l}\t{%1@SIZE, %k0|%k0, %1@SIZE}";
> +  else
> +    return "movabs{q}\t{%1@SIZE, %0|%0, %1@SIZE}";
> +}
> +  [(set_attr "type" "imov")
> +   (set_attr "mode" "<MODE>")])
> +
>  (include "mmx.md")
>  (include "sse.md")
>  (include "sync.md")
> diff --git a/gcc/config/i386/predicates.md b/gcc/config/i386/predicates.md
> index cd542b7..da01c9a 100644
> --- a/gcc/config/i386/predicates.md
> +++ b/gcc/config/i386/predicates.md
> @@ -124,6 +124,10 @@
>         (match_test "TARGET_64BIT")
>         (match_test "REGNO (op) > BX_REG")))
>
> +;; Return true if VALUE is symbol reference
> +(define_predicate "symbol_operand"
> +  (match_code "symbol_ref"))
> +
>  ;; Return true if VALUE can be stored in a sign extended immediate field.
>  (define_predicate "x86_64_immediate_operand"
>    (match_code "const_int,symbol_ref,label_ref,const")
> @@ -336,6 +340,19 @@
>    return false;
>  })
>
> +;; Return true if size of VALUE can be stored in a sign
> +;; extended immediate field.
> +(define_predicate "x86_64_immediate_size_operand"
> +  (match_code "symbol_ref")
> +{
> +  if (!TARGET_64BIT)
> +    return true;
> +
> +  /* For 64 bit target we may assume size of object fits
> +     immediate only when code model guarantees that.  */
> +  return (ix86_cmodel == CM_SMALL || ix86_cmodel == CM_KERNEL);
> +})
> +

This predicate causes bootstrap error:
predicates.md:362:38: error: unused parameter 'op' [-Werror=unused-parameter]

Would it be OK to work it around with a fake use?

if (GET_CODE (op) != SYMBOL_REF)
  gcc_unreachable ();

Thanks,
Ilya

>  ;; Return true if OP is general operand representable on x86_64.
>  (define_predicate "x86_64_general_operand"
>    (if_then_else (match_test "TARGET_64BIT")
Uros Bizjak Oct. 1, 2014, 3:17 p.m. UTC | #3 
On Wed, Oct 1, 2014 at 4:10 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:

>> +;; Return true if size of VALUE can be stored in a sign
>> +;; extended immediate field.
>> +(define_predicate "x86_64_immediate_size_operand"
>> +  (match_code "symbol_ref")
>> +{
>> +  if (!TARGET_64BIT)
>> +    return true;
>> +
>> +  /* For 64 bit target we may assume size of object fits
>> +     immediate only when code model guarantees that.  */
>> +  return (ix86_cmodel == CM_SMALL || ix86_cmodel == CM_KERNEL);
>> +})
>> +
>
> This predicate causes bootstrap error:
> predicates.md:362:38: error: unused parameter 'op' [-Werror=unused-parameter]

Huh? How is this predicate different from e.g.

(define_predicate "compare_operator"
  (match_code "compare"))

?

Can you please show generated code from gcc/insn-preds.c?

Uros.
Ilya Enkovich Oct. 1, 2014, 5:02 p.m. UTC | #4 
2014-10-01 19:17 GMT+04:00 Uros Bizjak <ubizjak@gmail.com>:
> On Wed, Oct 1, 2014 at 4:10 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>
>>> +;; Return true if size of VALUE can be stored in a sign
>>> +;; extended immediate field.
>>> +(define_predicate "x86_64_immediate_size_operand"
>>> +  (match_code "symbol_ref")
>>> +{
>>> +  if (!TARGET_64BIT)
>>> +    return true;
>>> +
>>> +  /* For 64 bit target we may assume size of object fits
>>> +     immediate only when code model guarantees that.  */
>>> +  return (ix86_cmodel == CM_SMALL || ix86_cmodel == CM_KERNEL);
>>> +})
>>> +
>>
>> This predicate causes bootstrap error:
>> predicates.md:362:38: error: unused parameter 'op' [-Werror=unused-parameter]
>
> Huh? How is this predicate different from e.g.
>
> (define_predicate "compare_operator"
>   (match_code "compare"))
>
> ?
>
> Can you please show generated code from gcc/insn-preds.c?
>
> Uros.

It is different because it has a code block which is used to generate
additional function. Here is what generated for the predicate:

static inline int
x86_64_immediate_size_operand_1 (rtx op, enum machine_mode mode
ATTRIBUTE_UNUSED)
{
  if (!TARGET_64BIT)
    return true;

  /* For 64 bit target we may assume size of object fits
     immediate only when code model guarantees that.  */
  return (ix86_cmodel == CM_SMALL || ix86_cmodel == CM_KERNEL);
}

int
x86_64_immediate_size_operand (rtx op, enum machine_mode mode ATTRIBUTE_UNUSED)
{
  return ((GET_CODE (op) == SYMBOL_REF) && (
(x86_64_immediate_size_operand_1 (op, mode)))) && (
(mode == VOIDmode || GET_MODE (op) == mode));
}


Ilya
Uros Bizjak Oct. 1, 2014, 5:57 p.m. UTC | #5 
On Wed, Oct 1, 2014 at 7:02 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2014-10-01 19:17 GMT+04:00 Uros Bizjak <ubizjak@gmail.com>:
>> On Wed, Oct 1, 2014 at 4:10 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>
>>>> +;; Return true if size of VALUE can be stored in a sign
>>>> +;; extended immediate field.
>>>> +(define_predicate "x86_64_immediate_size_operand"
>>>> +  (match_code "symbol_ref")
>>>> +{
>>>> +  if (!TARGET_64BIT)
>>>> +    return true;
>>>> +
>>>> +  /* For 64 bit target we may assume size of object fits
>>>> +     immediate only when code model guarantees that.  */
>>>> +  return (ix86_cmodel == CM_SMALL || ix86_cmodel == CM_KERNEL);
>>>> +})
>>>> +
>>>
>>> This predicate causes bootstrap error:
>>> predicates.md:362:38: error: unused parameter 'op' [-Werror=unused-parameter]
>>
>> Huh? How is this predicate different from e.g.
>>
>> (define_predicate "compare_operator"
>>   (match_code "compare"))
>>
>> ?
>>
>> Can you please show generated code from gcc/insn-preds.c?
>>
>> Uros.
>
> It is different because it has a code block which is used to generate
> additional function. Here is what generated for the predicate:
>
> static inline int
> x86_64_immediate_size_operand_1 (rtx op, enum machine_mode mode
> ATTRIBUTE_UNUSED)
> {
>   if (!TARGET_64BIT)
>     return true;
>
>   /* For 64 bit target we may assume size of object fits
>      immediate only when code model guarantees that.  */
>   return (ix86_cmodel == CM_SMALL || ix86_cmodel == CM_KERNEL);
> }
>
> int
> x86_64_immediate_size_operand (rtx op, enum machine_mode mode ATTRIBUTE_UNUSED)
> {
>   return ((GET_CODE (op) == SYMBOL_REF) && (
> (x86_64_immediate_size_operand_1 (op, mode)))) && (
> (mode == VOIDmode || GET_MODE (op) == mode));
> }

Well,

--cut here--
(define_predicate "x86_64_immediate_size_operand"
  (and (match_code "symbol_ref")
       (ior (not (match_test "TARGET_64BIT"))
            (and (match_test ("ix86_cmodel == CM_SMALL"))
             (match_test ("ix86_cmodel == CM_KERNEL"))))))
--cut here--

Uros.
diff mbox

Patch

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 2c367b2..db22b06 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -79,6 +79,7 @@ 
   UNSPEC_PLTOFF
   UNSPEC_MACHOPIC_OFFSET
   UNSPEC_PCREL
+  UNSPEC_SIZEOF
 
   ;; Prologue support
   UNSPEC_STACK_ALLOC
@@ -18554,6 +18555,21 @@ 
   "bndstx\t{%2, %3|%3, %2}"
   [(set_attr "type" "mpxst")])
 
+(define_insn "move_size_reloc_<mode>"
+  [(set (match_operand:SWI48 0 "register_operand" "=r")
+       (unspec:SWI48
+        [(match_operand:SWI48 1 "symbol_operand")]
+        UNSPEC_SIZEOF))]
+  "TARGET_MPX"
+{
+  if (x86_64_immediate_size_operand (operands[1], VOIDmode))
+    return "mov{l}\t{%1@SIZE, %k0|%k0, %1@SIZE}";
+  else
+    return "movabs{q}\t{%1@SIZE, %0|%0, %1@SIZE}";
+}
+  [(set_attr "type" "imov")
+   (set_attr "mode" "<MODE>")])
+
 (include "mmx.md")
 (include "sse.md")
 (include "sync.md")
diff --git a/gcc/config/i386/predicates.md b/gcc/config/i386/predicates.md
index cd542b7..da01c9a 100644
--- a/gcc/config/i386/predicates.md
+++ b/gcc/config/i386/predicates.md
@@ -124,6 +124,10 @@ 
        (match_test "TARGET_64BIT")
        (match_test "REGNO (op) > BX_REG")))
 
+;; Return true if VALUE is symbol reference
+(define_predicate "symbol_operand"
+  (match_code "symbol_ref"))
+
 ;; Return true if VALUE can be stored in a sign extended immediate field.
 (define_predicate "x86_64_immediate_operand"
   (match_code "const_int,symbol_ref,label_ref,const")
@@ -336,6 +340,19 @@ 
   return false;
 })
 
+;; Return true if size of VALUE can be stored in a sign
+;; extended immediate field.
+(define_predicate "x86_64_immediate_size_operand"
+  (match_code "symbol_ref")
+{
+  if (!TARGET_64BIT)
+    return true;
+
+  /* For 64 bit target we may assume size of object fits
+     immediate only when code model guarantees that.  */
+  return (ix86_cmodel == CM_SMALL || ix86_cmodel == CM_KERNEL);
+})
+
 ;; Return true if OP is general operand representable on x86_64.
 (define_predicate "x86_64_general_operand"
   (if_then_else (match_test "TARGET_64BIT")