diff mbox

[8/9] Add TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID

Message ID 1444280375-20866-9-git-send-email-rth@redhat.com
State New
Headers show

Commit Message

Richard Henderson Oct. 8, 2015, 4:59 a.m. UTC
* target.def (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): New.
	* targhooks.h (default_addr_space_zero_address_valid): Declare.
	* targhooks.c (default_addr_space_zero_address_valid): New.
	* doc/tm.texi, doc/tm.texi.in: Update.
	* config/i386/i386.c (ix86_addr_space_zero_address_valid): New.
	(TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): New.
	* fold-const.c (const_unop) [ADDR_SPACE_CONVERT_EXPR]: Disable
	folding of 0 when it is a valid address for the source space.
	* gimple.c (check_loadstore): Disable noticing dereference when
	0 is a valid address for the space.
---
 gcc/config/i386/i386.c | 10 ++++++++++
 gcc/doc/tm.texi        |  5 +++++
 gcc/doc/tm.texi.in     |  2 ++
 gcc/fold-const.c       |  6 +++++-
 gcc/gimple.c           | 12 +++++++++---
 gcc/target.def         |  9 +++++++++
 gcc/targhooks.c        |  9 +++++++++
 gcc/targhooks.h        |  1 +
 8 files changed, 50 insertions(+), 4 deletions(-)

Comments

Richard Biener Oct. 8, 2015, 10:20 a.m. UTC | #1
On Thu, Oct 8, 2015 at 6:59 AM, Richard Henderson <rth@redhat.com> wrote:
>         * target.def (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): New.
>         * targhooks.h (default_addr_space_zero_address_valid): Declare.
>         * targhooks.c (default_addr_space_zero_address_valid): New.
>         * doc/tm.texi, doc/tm.texi.in: Update.
>         * config/i386/i386.c (ix86_addr_space_zero_address_valid): New.
>         (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): New.
>         * fold-const.c (const_unop) [ADDR_SPACE_CONVERT_EXPR]: Disable
>         folding of 0 when it is a valid address for the source space.
>         * gimple.c (check_loadstore): Disable noticing dereference when
>         0 is a valid address for the space.

I think this is incomplete and you need to look at all places that check for
flag_delete_null_pointer_checks (ick, the ubsan abuse looks interesting...).
We'd best abstract that flag check somewhere, only doing the address-space
check for ! ADDR_SPACE_GENERIC.

I also wonder about the const_unop change - if the target address-space
has a valid 0 but the source has not then you create a valid object address
from an invalid one?

The check_loadstore change should instead have adjusted the
flag_delete_null_pointer_checks guard in infer_nonnull_range_by_dereference.

Yes, that flag is used to say whether 0 is a valid address ...

Richard.

> ---
>  gcc/config/i386/i386.c | 10 ++++++++++
>  gcc/doc/tm.texi        |  5 +++++
>  gcc/doc/tm.texi.in     |  2 ++
>  gcc/fold-const.c       |  6 +++++-
>  gcc/gimple.c           | 12 +++++++++---
>  gcc/target.def         |  9 +++++++++
>  gcc/targhooks.c        |  9 +++++++++
>  gcc/targhooks.h        |  1 +
>  8 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index f7abb00..e2dec2a 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -53669,6 +53669,16 @@ ix86_addr_space_convert (rtx op, tree from_type, tree to_type)
>  #undef TARGET_ADDR_SPACE_CONVERT
>  #define TARGET_ADDR_SPACE_CONVERT ix86_addr_space_convert
>
> +/* All use of segmentation is assumed to make address 0 valid.  */
> +
> +static bool
> +ix86_addr_space_zero_address_valid (addr_space_t as)
> +{
> +  return as != ADDR_SPACE_GENERIC;
> +}
> +#undef TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
> +#define TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID ix86_addr_space_zero_address_valid
> +
>  /* Initialize the GCC target structure.  */
>  #undef TARGET_RETURN_IN_MEMORY
>  #define TARGET_RETURN_IN_MEMORY ix86_return_in_memory
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index 72366b9..238f5f5 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -10312,6 +10312,11 @@ arithmetic operations.  Pointers to a superset address space can be
>  converted to pointers to a subset address space via explicit casts.
>  @end deftypefn
>
> +@deftypefn {Target Hook} bool TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID (addr_space_t @var{as})
> +Define this to modify the default handling of address 0 for the
> +address space.  Return true if 0 should be considered a valid address.
> +@end deftypefn
> +
>  @deftypefn {Target Hook} rtx TARGET_ADDR_SPACE_CONVERT (rtx @var{op}, tree @var{from_type}, tree @var{to_type})
>  Define this to convert the pointer expression represented by the RTL
>  @var{op} with type @var{from_type} that points to a named address
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index d8d0087..9230fb2 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -7432,6 +7432,8 @@ c_register_addr_space ("__ea", ADDR_SPACE_EA);
>
>  @hook TARGET_ADDR_SPACE_SUBSET_P
>
> +@hook TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
> +
>  @hook TARGET_ADDR_SPACE_CONVERT
>
>  @node Misc
> diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> index 2851a29..2e06217 100644
> --- a/gcc/fold-const.c
> +++ b/gcc/fold-const.c
> @@ -1564,7 +1564,11 @@ const_unop (enum tree_code code, tree type, tree arg0)
>        return fold_convert_const (code, type, arg0);
>
>      case ADDR_SPACE_CONVERT_EXPR:
> -      if (integer_zerop (arg0))
> +      /* If the source address is 0, and the source address space
> +        cannot have a valid object at 0, fold to dest type null.  */
> +      if (integer_zerop (arg0)
> +         && !(targetm.addr_space.zero_address_valid
> +              (TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg0))))))
>         return fold_convert_const (code, type, arg0);
>        break;
>
> diff --git a/gcc/gimple.c b/gcc/gimple.c
> index c3762e1..e551dac 100644
> --- a/gcc/gimple.c
> +++ b/gcc/gimple.c
> @@ -2618,9 +2618,15 @@ nonfreeing_call_p (gimple *call)
>  static bool
>  check_loadstore (gimple *, tree op, tree, void *data)
>  {
> -  if ((TREE_CODE (op) == MEM_REF || TREE_CODE (op) == TARGET_MEM_REF)
> -      && operand_equal_p (TREE_OPERAND (op, 0), (tree)data, 0))
> -    return true;
> +  if (TREE_CODE (op) == MEM_REF || TREE_CODE (op) == TARGET_MEM_REF)
> +    {
> +      /* Some address spaces may legitimately dereference zero.  */
> +      addr_space_t as = TYPE_ADDR_SPACE (TREE_TYPE (op));
> +      if (targetm.addr_space.zero_address_valid (as))
> +       return false;
> +
> +      return operand_equal_p (TREE_OPERAND (op, 0), (tree)data, 0);
> +    }
>    return false;
>  }
>
> diff --git a/gcc/target.def b/gcc/target.def
> index d29aad5..f75ce97 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -3185,6 +3185,15 @@ converted to pointers to a subset address space via explicit casts.",
>   bool, (addr_space_t subset, addr_space_t superset),
>   default_addr_space_subset_p)
>
> +/* True if 0 is a valid address in the address space, or false if
> +   0 is a NULL in the address space.  */
> +DEFHOOK
> +(zero_address_valid,
> + "Define this to modify the default handling of address 0 for the\n\
> +address space.  Return true if 0 should be considered a valid address.",
> + bool, (addr_space_t as),
> + default_addr_space_zero_address_valid)
> +
>  /* Function to convert an rtl expression from one address space to another.  */
>  DEFHOOK
>  (convert,
> diff --git a/gcc/targhooks.c b/gcc/targhooks.c
> index a8a243c..c78a540 100644
> --- a/gcc/targhooks.c
> +++ b/gcc/targhooks.c
> @@ -1256,6 +1256,15 @@ default_addr_space_subset_p (addr_space_t subset, addr_space_t superset)
>    return (subset == superset);
>  }
>
> +/* The default hook for determining if 0 within a named address
> +   space is a valid address.  */
> +
> +bool
> +default_addr_space_zero_address_valid (addr_space_t as ATTRIBUTE_UNUSED)
> +{
> +  return false;
> +}
> +
>  /* The default hook for TARGET_ADDR_SPACE_CONVERT. This hook should never be
>     called for targets with only a generic address space.  */
>
> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
> index 77c284a..ade3327 100644
> --- a/gcc/targhooks.h
> +++ b/gcc/targhooks.h
> @@ -173,6 +173,7 @@ extern bool default_addr_space_legitimate_address_p (machine_mode, rtx,
>  extern rtx default_addr_space_legitimize_address (rtx, rtx, machine_mode,
>                                                   addr_space_t);
>  extern bool default_addr_space_subset_p (addr_space_t, addr_space_t);
> +extern bool default_addr_space_zero_address_valid (addr_space_t);
>  extern rtx default_addr_space_convert (rtx, tree, tree);
>  extern unsigned int default_case_values_threshold (void);
>  extern bool default_have_conditional_execution (void);
> --
> 2.4.3
>
Richard Henderson Oct. 8, 2015, 9:10 p.m. UTC | #2
On 10/08/2015 09:20 PM, Richard Biener wrote:
> On Thu, Oct 8, 2015 at 6:59 AM, Richard Henderson <rth@redhat.com> wrote:
>>          * target.def (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): New.
>>          * targhooks.h (default_addr_space_zero_address_valid): Declare.
>>          * targhooks.c (default_addr_space_zero_address_valid): New.
>>          * doc/tm.texi, doc/tm.texi.in: Update.
>>          * config/i386/i386.c (ix86_addr_space_zero_address_valid): New.
>>          (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): New.
>>          * fold-const.c (const_unop) [ADDR_SPACE_CONVERT_EXPR]: Disable
>>          folding of 0 when it is a valid address for the source space.
>>          * gimple.c (check_loadstore): Disable noticing dereference when
>>          0 is a valid address for the space.
>
> I think this is incomplete and you need to look at all places that check for
> flag_delete_null_pointer_checks (ick, the ubsan abuse looks interesting...).
> We'd best abstract that flag check somewhere, only doing the address-space
> check for ! ADDR_SPACE_GENERIC.

I did a fair survey of the uses of f_d_n_p_c.  Most of them are tests for 
explicit symbols that are weak, etc.  I suppose we should probably then check 
to see if the symbol is placed in a non-default address space, but in the 
context of the code I was working on, that never comes up.

> I also wonder about the const_unop change - if the target address-space
> has a valid 0 but the source has not then you create a valid object address
> from an invalid one?

I guess we would, but ... what else can you do when there's no invalid address?

> The check_loadstore change should instead have adjusted the
> flag_delete_null_pointer_checks guard in infer_nonnull_range_by_dereference.

Nope, that doesn't work.  You have to wait until you see the actual MEM being 
dereferenced before you can look at it's address space.


r~
Richard Biener Oct. 12, 2015, 10:10 a.m. UTC | #3
On Thu, Oct 8, 2015 at 11:10 PM, Richard Henderson <rth@redhat.com> wrote:
> On 10/08/2015 09:20 PM, Richard Biener wrote:
>>
>> On Thu, Oct 8, 2015 at 6:59 AM, Richard Henderson <rth@redhat.com> wrote:
>>>
>>>          * target.def (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): New.
>>>          * targhooks.h (default_addr_space_zero_address_valid): Declare.
>>>          * targhooks.c (default_addr_space_zero_address_valid): New.
>>>          * doc/tm.texi, doc/tm.texi.in: Update.
>>>          * config/i386/i386.c (ix86_addr_space_zero_address_valid): New.
>>>          (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): New.
>>>          * fold-const.c (const_unop) [ADDR_SPACE_CONVERT_EXPR]: Disable
>>>          folding of 0 when it is a valid address for the source space.
>>>          * gimple.c (check_loadstore): Disable noticing dereference when
>>>          0 is a valid address for the space.
>>
>>
>> I think this is incomplete and you need to look at all places that check
>> for
>> flag_delete_null_pointer_checks (ick, the ubsan abuse looks
>> interesting...).
>> We'd best abstract that flag check somewhere, only doing the address-space
>> check for ! ADDR_SPACE_GENERIC.
>
>
> I did a fair survey of the uses of f_d_n_p_c.  Most of them are tests for
> explicit symbols that are weak, etc.  I suppose we should probably then
> check to see if the symbol is placed in a non-default address space, but in
> the context of the code I was working on, that never comes up.

One I know about is tree-ssa-structalias.c:get_constraint_for_1 which treats
zero address specially.  A zero_address_valid_p predicate taking a
pointer (type)
would be nice to have to abstract those flag checks appropriately.

>> I also wonder about the const_unop change - if the target address-space
>> has a valid 0 but the source has not then you create a valid object
>> address
>> from an invalid one?
>
>
> I guess we would, but ... what else can you do when there's no invalid
> address?

True ... maybe a less likely valid one?  (0xfff...ff?)

>> The check_loadstore change should instead have adjusted the
>> flag_delete_null_pointer_checks guard in
>> infer_nonnull_range_by_dereference.
>
>
> Nope, that doesn't work.  You have to wait until you see the actual MEM
> being dereferenced before you can look at it's address space.

Well, as we are explicitely looking for the pointer 'op' we know the
address-space
beforehand, no?  TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (op)))?

Richard.

>
> r~
Richard Henderson Oct. 12, 2015, 11:27 p.m. UTC | #4
On 10/12/2015 09:10 PM, Richard Biener wrote:
>>> The check_loadstore change should instead have adjusted the
>>> flag_delete_null_pointer_checks guard in
>>> infer_nonnull_range_by_dereference.
>>
>>
>> Nope, that doesn't work.  You have to wait until you see the actual MEM
>> being dereferenced before you can look at it's address space.
>
> Well, as we are explicitely looking for the pointer 'op' we know the
> address-space
> beforehand, no?  TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (op)))?

No.  We don't even know what type we're looking for; we're merely looking for 
any use of NULL within any memory reference within STMT.

Specifically, when we're not looking for a specific SSA_NAME (which would be 
properly typed), we always pass in a plain (void *)0:

           bool by_dereference
             = infer_nonnull_range_by_dereference (stmt, null_pointer_node);



r~
Richard Biener Oct. 13, 2015, 10:13 a.m. UTC | #5
On Tue, Oct 13, 2015 at 1:27 AM, Richard Henderson <rth@redhat.com> wrote:
> On 10/12/2015 09:10 PM, Richard Biener wrote:
>>>>
>>>> The check_loadstore change should instead have adjusted the
>>>> flag_delete_null_pointer_checks guard in
>>>> infer_nonnull_range_by_dereference.
>>>
>>>
>>>
>>> Nope, that doesn't work.  You have to wait until you see the actual MEM
>>> being dereferenced before you can look at it's address space.
>>
>>
>> Well, as we are explicitely looking for the pointer 'op' we know the
>> address-space
>> beforehand, no?  TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (op)))?
>
>
> No.  We don't even know what type we're looking for; we're merely looking
> for any use of NULL within any memory reference within STMT.
>
> Specifically, when we're not looking for a specific SSA_NAME (which would be
> properly typed), we always pass in a plain (void *)0:
>
>           bool by_dereference
>             = infer_nonnull_range_by_dereference (stmt, null_pointer_node);

Ick.

Richard.

>
>
> r~
Jeff Law Oct. 13, 2015, 3:49 p.m. UTC | #6
On 10/13/2015 04:13 AM, Richard Biener wrote:
> On Tue, Oct 13, 2015 at 1:27 AM, Richard Henderson <rth@redhat.com> wrote:
>> On 10/12/2015 09:10 PM, Richard Biener wrote:
>>>>>
>>>>> The check_loadstore change should instead have adjusted the
>>>>> flag_delete_null_pointer_checks guard in
>>>>> infer_nonnull_range_by_dereference.
>>>>
>>>>
>>>>
>>>> Nope, that doesn't work.  You have to wait until you see the actual MEM
>>>> being dereferenced before you can look at it's address space.
>>>
>>>
>>> Well, as we are explicitely looking for the pointer 'op' we know the
>>> address-space
>>> beforehand, no?  TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (op)))?
>>
>>
>> No.  We don't even know what type we're looking for; we're merely looking
>> for any use of NULL within any memory reference within STMT.
>>
>> Specifically, when we're not looking for a specific SSA_NAME (which would be
>> properly typed), we always pass in a plain (void *)0:
>>
>>            bool by_dereference
>>              = infer_nonnull_range_by_dereference (stmt, null_pointer_node);
>
> Ick.
It's just looking to see if there's an explicit *0 in stmt.  That can 
occur due to cprop & friends obviously.  It was an easy way to avoid 
having to write a special walker.

The problem here is we don't know what address space the *0 is going to 
hit, right?   Isn't that also an issue for code generation as well?

Jeff
Richard Henderson Oct. 13, 2015, 8:59 p.m. UTC | #7
On 10/14/2015 02:49 AM, Jeff Law wrote:
> The problem here is we don't know what address space the *0 is going to hit,
> right?

Correct, not before we do the walk of stmt to see what's present.

> Isn't that also an issue for code generation as well?

What sort of problem are you thinking of?  I haven't seen one yet.


r~
Richard Biener Oct. 14, 2015, 9:19 a.m. UTC | #8
On Tue, Oct 13, 2015 at 10:59 PM, Richard Henderson <rth@redhat.com> wrote:
> On 10/14/2015 02:49 AM, Jeff Law wrote:
>>
>> The problem here is we don't know what address space the *0 is going to
>> hit,
>> right?
>
>
> Correct, not before we do the walk of stmt to see what's present.
>
>> Isn't that also an issue for code generation as well?
>
>
> What sort of problem are you thinking of?  I haven't seen one yet.

The actual dereference of course has a properly address-space qualified zero.

Only your walking depends on operand_equal_p to treat different address-space
zero addresses as equal (which they are of course not ...):


int
operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
{
...
  /* Check equality of integer constants before bailing out due to
     precision differences.  */
  if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
    {
      /* Address of INTEGER_CST is not defined; check that we did not forget
         to drop the OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags.  */
      gcc_checking_assert (!(flags
                             & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF)));
      return tree_int_cst_equal (arg0, arg1);
    }

but only later we do

      /* We cannot consider pointers to different address space equal.  */
      if (POINTER_TYPE_P (TREE_TYPE (arg0))
                          && POINTER_TYPE_P (TREE_TYPE (arg1))
          && (TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg0)))
              != TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg1)))))
        return 0;

So "fixing" that would make the walker only look for default
address-space zero dereferences.

I think we need to fix operand_equal_p anyway because 0 is clearly not
equal to 0 (only if
they convert to the same literal)

Richard.


>
> r~
Richard Biener Oct. 14, 2015, 9:22 a.m. UTC | #9
On Wed, Oct 14, 2015 at 11:19 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Tue, Oct 13, 2015 at 10:59 PM, Richard Henderson <rth@redhat.com> wrote:
>> On 10/14/2015 02:49 AM, Jeff Law wrote:
>>>
>>> The problem here is we don't know what address space the *0 is going to
>>> hit,
>>> right?
>>
>>
>> Correct, not before we do the walk of stmt to see what's present.
>>
>>> Isn't that also an issue for code generation as well?
>>
>>
>> What sort of problem are you thinking of?  I haven't seen one yet.
>
> The actual dereference of course has a properly address-space qualified zero.
>
> Only your walking depends on operand_equal_p to treat different address-space
> zero addresses as equal (which they are of course not ...):
>
>
> int
> operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
> {
> ...
>   /* Check equality of integer constants before bailing out due to
>      precision differences.  */
>   if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
>     {
>       /* Address of INTEGER_CST is not defined; check that we did not forget
>          to drop the OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags.  */
>       gcc_checking_assert (!(flags
>                              & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF)));
>       return tree_int_cst_equal (arg0, arg1);
>     }
>
> but only later we do
>
>       /* We cannot consider pointers to different address space equal.  */
>       if (POINTER_TYPE_P (TREE_TYPE (arg0))
>                           && POINTER_TYPE_P (TREE_TYPE (arg1))
>           && (TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg0)))
>               != TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg1)))))
>         return 0;
>
> So "fixing" that would make the walker only look for default
> address-space zero dereferences.
>
> I think we need to fix operand_equal_p anyway because 0 is clearly not
> equal to 0 (only if
> they convert to the same literal)

I think you could trigger bogus CSE of dereferences of literal addresses
from different address-spaces.

Richard.

> Richard.
>
>
>>
>> r~
Jeff Law Oct. 14, 2015, 3:22 p.m. UTC | #10
On 10/13/2015 02:59 PM, Richard Henderson wrote:
> On 10/14/2015 02:49 AM, Jeff Law wrote:
>> The problem here is we don't know what address space the *0 is going
>> to hit,
>> right?
>
> Correct, not before we do the walk of stmt to see what's present.
So the address space information isn't part of the address?  I must 
admit I haven't looked at how that stuff is being implemented.

>
>> Isn't that also an issue for code generation as well?
>
> What sort of problem are you thinking of?  I haven't seen one yet.
If the address space information was supposed to be carried in the 
address itself, then we'd need the address to be distinct from 
NULL_POINTER_NODE.

It sounds to me like you're carrying address space information outside 
the address itself, which avoid those issues.  However, it does mean 
that the path isolation code needs some kind of adjustment to 
distinguish between *0 that will fault and *0 which hits a different 
address space and may not fault.

jeff
Jeff Law Oct. 14, 2015, 3:28 p.m. UTC | #11
On 10/14/2015 03:19 AM, Richard Biener wrote:
> On Tue, Oct 13, 2015 at 10:59 PM, Richard Henderson <rth@redhat.com> wrote:
>> On 10/14/2015 02:49 AM, Jeff Law wrote:
>>>
>>> The problem here is we don't know what address space the *0 is going to
>>> hit,
>>> right?
>>
>>
>> Correct, not before we do the walk of stmt to see what's present.
>>
>>> Isn't that also an issue for code generation as well?
>>
>>
>> What sort of problem are you thinking of?  I haven't seen one yet.
>
> The actual dereference of course has a properly address-space qualified zero.
OK.  That's the bit I was missing and hinted out in the message I just 
sent -- the address-space information is carried outside the address. 
It seems that we're carrying it in the type, which is probably sensible 
at some level.


>
> Only your walking depends on operand_equal_p to treat different address-space
> zero addresses as equal (which they are of course not ...):
And that's the key problem with carrying the address-space information 
outside the address.   We have to look at more than just the raw address 
to determine if we've got a faulting *0 vs an address space qualified *0.

>
>
> int
> operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
> {
> ...
>    /* Check equality of integer constants before bailing out due to
>       precision differences.  */
>    if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
>      {
>        /* Address of INTEGER_CST is not defined; check that we did not forget
>           to drop the OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags.  */
>        gcc_checking_assert (!(flags
>                               & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF)));
>        return tree_int_cst_equal (arg0, arg1);
>      }
>
> but only later we do
>
>        /* We cannot consider pointers to different address space equal.  */
>        if (POINTER_TYPE_P (TREE_TYPE (arg0))
>                            && POINTER_TYPE_P (TREE_TYPE (arg1))
>            && (TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg0)))
>                != TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg1)))))
>          return 0;
>
> So "fixing" that would make the walker only look for default
> address-space zero dereferences.
Agreed.

>
> I think we need to fix operand_equal_p anyway because 0 is clearly not
> equal to 0 (only if they convert to the same literal)
My worry here is we'd be getting onto a slippery slope.  But it may be 
unavoidable.

jeff
Richard Henderson Oct. 14, 2015, 9:20 p.m. UTC | #12
On 10/14/2015 08:22 PM, Richard Biener wrote:
> I think you could trigger bogus CSE of dereferences of literal addresses
> from different address-spaces.

Good catch.  You're spot on with that.


r~


int test(void)
{
   int __seg_fs *f = (int __seg_fs *)16;
   int __seg_gs *g = (int __seg_gs *)16;
   return *f + *g;
}

test:
	movl	%fs:16, %eax
	addl	%eax, %eax
	ret
diff mbox

Patch

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index f7abb00..e2dec2a 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -53669,6 +53669,16 @@  ix86_addr_space_convert (rtx op, tree from_type, tree to_type)
 #undef TARGET_ADDR_SPACE_CONVERT
 #define TARGET_ADDR_SPACE_CONVERT ix86_addr_space_convert
 
+/* All use of segmentation is assumed to make address 0 valid.  */
+
+static bool
+ix86_addr_space_zero_address_valid (addr_space_t as)
+{
+  return as != ADDR_SPACE_GENERIC;
+}
+#undef TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
+#define TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID ix86_addr_space_zero_address_valid
+
 /* Initialize the GCC target structure.  */
 #undef TARGET_RETURN_IN_MEMORY
 #define TARGET_RETURN_IN_MEMORY ix86_return_in_memory
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 72366b9..238f5f5 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -10312,6 +10312,11 @@  arithmetic operations.  Pointers to a superset address space can be
 converted to pointers to a subset address space via explicit casts.
 @end deftypefn
 
+@deftypefn {Target Hook} bool TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID (addr_space_t @var{as})
+Define this to modify the default handling of address 0 for the
+address space.  Return true if 0 should be considered a valid address.
+@end deftypefn
+
 @deftypefn {Target Hook} rtx TARGET_ADDR_SPACE_CONVERT (rtx @var{op}, tree @var{from_type}, tree @var{to_type})
 Define this to convert the pointer expression represented by the RTL
 @var{op} with type @var{from_type} that points to a named address
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index d8d0087..9230fb2 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -7432,6 +7432,8 @@  c_register_addr_space ("__ea", ADDR_SPACE_EA);
 
 @hook TARGET_ADDR_SPACE_SUBSET_P
 
+@hook TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID
+
 @hook TARGET_ADDR_SPACE_CONVERT
 
 @node Misc
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 2851a29..2e06217 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -1564,7 +1564,11 @@  const_unop (enum tree_code code, tree type, tree arg0)
       return fold_convert_const (code, type, arg0);
 
     case ADDR_SPACE_CONVERT_EXPR:
-      if (integer_zerop (arg0))
+      /* If the source address is 0, and the source address space
+	 cannot have a valid object at 0, fold to dest type null.  */
+      if (integer_zerop (arg0)
+	  && !(targetm.addr_space.zero_address_valid
+	       (TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg0))))))
 	return fold_convert_const (code, type, arg0);
       break;
 
diff --git a/gcc/gimple.c b/gcc/gimple.c
index c3762e1..e551dac 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -2618,9 +2618,15 @@  nonfreeing_call_p (gimple *call)
 static bool
 check_loadstore (gimple *, tree op, tree, void *data)
 {
-  if ((TREE_CODE (op) == MEM_REF || TREE_CODE (op) == TARGET_MEM_REF)
-      && operand_equal_p (TREE_OPERAND (op, 0), (tree)data, 0))
-    return true;
+  if (TREE_CODE (op) == MEM_REF || TREE_CODE (op) == TARGET_MEM_REF)
+    {
+      /* Some address spaces may legitimately dereference zero.  */
+      addr_space_t as = TYPE_ADDR_SPACE (TREE_TYPE (op));
+      if (targetm.addr_space.zero_address_valid (as))
+	return false;
+
+      return operand_equal_p (TREE_OPERAND (op, 0), (tree)data, 0);
+    }
   return false;
 }
 
diff --git a/gcc/target.def b/gcc/target.def
index d29aad5..f75ce97 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -3185,6 +3185,15 @@  converted to pointers to a subset address space via explicit casts.",
  bool, (addr_space_t subset, addr_space_t superset),
  default_addr_space_subset_p)
 
+/* True if 0 is a valid address in the address space, or false if
+   0 is a NULL in the address space.  */
+DEFHOOK
+(zero_address_valid,
+ "Define this to modify the default handling of address 0 for the\n\
+address space.  Return true if 0 should be considered a valid address.",
+ bool, (addr_space_t as),
+ default_addr_space_zero_address_valid)
+
 /* Function to convert an rtl expression from one address space to another.  */
 DEFHOOK
 (convert,
diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index a8a243c..c78a540 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -1256,6 +1256,15 @@  default_addr_space_subset_p (addr_space_t subset, addr_space_t superset)
   return (subset == superset);
 }
 
+/* The default hook for determining if 0 within a named address
+   space is a valid address.  */
+
+bool
+default_addr_space_zero_address_valid (addr_space_t as ATTRIBUTE_UNUSED)
+{
+  return false;
+}
+
 /* The default hook for TARGET_ADDR_SPACE_CONVERT. This hook should never be
    called for targets with only a generic address space.  */
 
diff --git a/gcc/targhooks.h b/gcc/targhooks.h
index 77c284a..ade3327 100644
--- a/gcc/targhooks.h
+++ b/gcc/targhooks.h
@@ -173,6 +173,7 @@  extern bool default_addr_space_legitimate_address_p (machine_mode, rtx,
 extern rtx default_addr_space_legitimize_address (rtx, rtx, machine_mode,
 						  addr_space_t);
 extern bool default_addr_space_subset_p (addr_space_t, addr_space_t);
+extern bool default_addr_space_zero_address_valid (addr_space_t);
 extern rtx default_addr_space_convert (rtx, tree, tree);
 extern unsigned int default_case_values_threshold (void);
 extern bool default_have_conditional_execution (void);