diff mbox

Fix ivopts address space confusion

Message ID 53E3D877.4050607@codesourcery.com
State New
Headers show

Commit Message

Bernd Schmidt Aug. 7, 2014, 7:50 p.m. UTC
This is a followup fix for the patches from PR41857, which contained a 
bug that is exposed by the ptx backend. The problem is finding the right 
pointer type for a mem_ref during ivopts.

The original problem was that on spu-elf, __ea pointers are 64 bit while 
sizetype is 32 bit. During ivopts, pointer types are typically replaced 
by unsigned integers in computations, and in the original testcase we 
have an aff_tree containing only one long long element, but no pointer 
base, and addr_to_parts casts everything to sizetype, losing parts of 
the address.

To fix that, code was introduced to promote one of the elements of the 
aff_tree to become the base, cast it to a pointer of an appropriate 
type, and thus avoid the conversion to sizetype for that element. This 
element is the base_hint, which is chosen based on the candidate's 
base_object.

The problem lies in the following code in move_hint_to_base:

   /* Cast value to appropriate pointer type.  We cannot use a pointer
      to TYPE directly, as the back-end will assume registers of pointer
      type are aligned, and just the base itself may not actually be.
      We use void pointer to the type's address space instead.  */
   qual = ENCODE_QUAL_ADDR_SPACE (TYPE_ADDR_SPACE (type));
   type = build_qualified_type (void_type_node, qual);
   parts->base = fold_convert (build_pointer_type (type), val);

This is confused about address spaces. We can get here with either an 
integer (for the the original testcase in 4.5, it's a long long that 
holds the pointer value), but I think we can also arrive here with a 
pointer. Assuming a normal machine, if it's a long long created by 
ivopts, TYPE_ADDR_SPACE should be just generic, and if it's some other 
pointer, we should be looking at TYPE_ADDR_SPACE (TREE_TYPE (type)). 
Thus, the code seems essentially unnecessary, but - up to this point - 
harmless.

On ptx however, local variables have an address space, and the above 
code picks that up and incorrectly places it on the pointer destination 
type.

The following patch reworks this area - instead of trying to find a 
proper pointer type, it just recognizes the case where an integer is 
promoted to be the base, and performs all calculations in that type 
rather than sizetype. That also seems to be an additional bugfix over 
the original change, which could still lose address bits in the index. 
I've verified that this produces the same assembly for the original 
testcase on spu-elf with the gcc-4_5-branch, and it solves the problems 
I was seeing on ptx.

Bootstrapped and tested on x86_64-linux. Ok?


Bernd

Comments

Bernd Schmidt Aug. 7, 2014, 8:31 p.m. UTC | #1
On 08/07/2014 09:50 PM, Bernd Schmidt wrote:
> The following patch reworks this area - instead of trying to find a
> proper pointer type, it just recognizes the case where an integer is
> promoted to be the base, and performs all calculations in that type
> rather than sizetype. That also seems to be an additional bugfix over
> the original change, which could still lose address bits in the index.
> I've verified that this produces the same assembly for the original
> testcase on spu-elf with the gcc-4_5-branch, and it solves the problems
> I was seeing on ptx.

Argh, not quite. It fixes one class of testcases, but also introduces 
some new failures. New patch tomorrow or so.


Bernd
diff mbox

Patch

	* tree-ssa-address.c (move_hint_to_base): Remove arg TYPE.  All
	callers changed.  Don't cast the new base to a pointer type.
	(addr_to_parts): If the base has an integer type, use that instead
	of sizetype for calculations.

Index: gcc/tree-ssa-address.c
===================================================================
--- gcc/tree-ssa-address.c	(revision 436508)
+++ gcc/tree-ssa-address.c	(working copy)
@@ -443,12 +443,10 @@  move_fixed_address_to_symbol (struct mem
 /* If ADDR contains an instance of BASE_HINT, move it to PARTS->base.  */
 
 static void
-move_hint_to_base (tree type, struct mem_address *parts, tree base_hint,
-		   aff_tree *addr)
+move_hint_to_base (struct mem_address *parts, tree base_hint, aff_tree *addr)
 {
   unsigned i;
   tree val = NULL_TREE;
-  int qual;
 
   for (i = 0; i < addr->n; i++)
     {
@@ -463,13 +461,7 @@  move_hint_to_base (tree type, struct mem
   if (i == addr->n)
     return;
 
-  /* Cast value to appropriate pointer type.  We cannot use a pointer
-     to TYPE directly, as the back-end will assume registers of pointer
-     type are aligned, and just the base itself may not actually be.
-     We use void pointer to the type's address space instead.  */
-  qual = ENCODE_QUAL_ADDR_SPACE (TYPE_ADDR_SPACE (type));
-  type = build_qualified_type (void_type_node, qual);
-  parts->base = fold_convert (build_pointer_type (type), val);
+  parts->base = val;
   aff_combination_remove_elt (addr, i);
 }
 
@@ -639,7 +631,7 @@  addr_to_parts (tree type, aff_tree *addr
 	       tree base_hint, struct mem_address *parts,
                bool speed)
 {
-  tree part;
+  tree part, int_type;
   unsigned i;
 
   parts->symbol = NULL_TREE;
@@ -671,21 +663,29 @@  addr_to_parts (tree type, aff_tree *addr
      there is no reliable way how to distinguish between pointer and its
      offset, this is just a guess.  */
   if (!parts->symbol && base_hint)
-    move_hint_to_base (type, parts, base_hint, addr);
+    move_hint_to_base (parts, base_hint, addr);
   if (!parts->symbol && !parts->base)
     move_pointer_to_base (parts, addr);
 
+  /* The call to move_hint_to_base may have promoted an integer to the base.
+     Since addresses in different address spaces can have different sizes, do
+     not cast to sizetype in that case, but use the type of the base.  */
+  if (parts->base && TREE_CODE (TREE_TYPE (parts->base)) == INTEGER_TYPE)
+    int_type = TREE_TYPE (parts->base);
+  else
+    int_type = sizetype;
+
   /* Then try to process the remaining elements.  */
   for (i = 0; i < addr->n; i++)
     {
-      part = fold_convert (sizetype, addr->elts[i].val);
+      part = fold_convert (int_type, addr->elts[i].val);
       if (addr->elts[i].coef != 1)
-	part = fold_build2 (MULT_EXPR, sizetype, part,
-			    wide_int_to_tree (sizetype, addr->elts[i].coef));
+	part = fold_build2 (MULT_EXPR, int_type, part,
+			    wide_int_to_tree (int_type, addr->elts[i].coef));
       add_to_parts (parts, part);
     }
   if (addr->rest)
-    add_to_parts (parts, fold_convert (sizetype, addr->rest));
+    add_to_parts (parts, fold_convert (int_type, addr->rest));
 }
 
 /* Force the PARTS to register.  */