diff mbox

[sparc] : Use ROUND_UP and ROUND_DOWN macros

Message ID CAFULd4atu0+WjdR29UxanR0MifhrQPRjkyGse7V_btwgaQua7A@mail.gmail.com
State New
Headers show

Commit Message

Uros Bizjak Oct. 12, 2015, 5:01 p.m. UTC 
Two functional changes I'd like to point out:

 /* ALIGN FRAMES on double word boundaries */
-#define SPARC_STACK_ALIGN(LOC) \
-  (TARGET_ARCH64 ? (((LOC)+15) & ~15) : (((LOC)+7) & ~7))
+#define SPARC_STACK_ALIGN(LOC) ROUND_UP ((LOC), UNITS_PER_WORD * 2)

The one above uses UNITS_PER_WORD in stack alignment calculation

       /* Always preserve double-word alignment.  */
-      offset = (offset + 8) & -8;
+      offset = ROUND_UP (offset, 8);

The one above looks like off-by-one bug, but this needs a confirmation.

2015-10-12  Uros Bizjak  <ubizjak@gmail.com>

    * config/sparc/sparc.h (SPARC_STACK_ALIGN): Implement using
    ROUND_UP macro and UNITS_PER_WORD * 2.
    * config/sparc/sparc.c (sparc_compute_frame_size):
    Use ROUND_UP and ROUND_DOWN macros where applicable.
    (function_arg_record_value, function_arg_record_value_1)
    (function_arg_record_value_1): Ditto.
    (emit_save_or_restore_regs): Use ROUND_UP to preserve offset
    alignment to double-word.
    (sparc_gimplify_va_arg): Use ROUND_UP ro calculate rsize.
    (sparc_emit_probe_stack_range): Use ROUND_DOWN to calculate
    rounded_size.

Tested by building a crosscompiler to sparc-linux-gnu. Due to the two
above changes, can someone please bootstrap and regression test this
patch properly on sparc targets?

OK for mainline if bootstrap+regtest show no problems?

Uros.

Comments

Eric Botcazou Oct. 13, 2015, 10:10 a.m. UTC | #1 
> Two functional changes I'd like to point out:
> 
>  /* ALIGN FRAMES on double word boundaries */
> -#define SPARC_STACK_ALIGN(LOC) \
> -  (TARGET_ARCH64 ? (((LOC)+15) & ~15) : (((LOC)+7) & ~7))
> +#define SPARC_STACK_ALIGN(LOC) ROUND_UP ((LOC), UNITS_PER_WORD * 2)
> 
> The one above uses UNITS_PER_WORD in stack alignment calculation

OK.

>        /* Always preserve double-word alignment.  */
> -      offset = (offset + 8) & -8;
> +      offset = ROUND_UP (offset, 8);
> 
> The one above looks like off-by-one bug, but this needs a confirmation.

No, it's correct, it's a bump of 8 followed by a ROUND_DOWN (the offset may or 
may not have been bumped by 4 already in the code just above).
Uros Bizjak Oct. 13, 2015, 10:30 a.m. UTC | #2 
On Tue, Oct 13, 2015 at 12:10 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Two functional changes I'd like to point out:
>>
>>  /* ALIGN FRAMES on double word boundaries */
>> -#define SPARC_STACK_ALIGN(LOC) \
>> -  (TARGET_ARCH64 ? (((LOC)+15) & ~15) : (((LOC)+7) & ~7))
>> +#define SPARC_STACK_ALIGN(LOC) ROUND_UP ((LOC), UNITS_PER_WORD * 2)
>>
>> The one above uses UNITS_PER_WORD in stack alignment calculation
>
> OK.
>
>>        /* Always preserve double-word alignment.  */
>> -      offset = (offset + 8) & -8;
>> +      offset = ROUND_UP (offset, 8);
>>
>> The one above looks like off-by-one bug, but this needs a confirmation.
>
> No, it's correct, it's a bump of 8 followed by a ROUND_DOWN (the offset may or
> may not have been bumped by 4 already in the code just above).

In this case, I think it is better to write this part as:

--cut here--
offset += 8;

/* Always preserve double-word alignment.  */
offset = ROUND_DOWN (offset, 8);
--cut here--

WDYT?

Uros.
Eric Botcazou Oct. 13, 2015, 10:41 a.m. UTC | #3 
> In this case, I think it is better to write this part as:
> 
> --cut here--
> offset += 8;
> 
> /* Always preserve double-word alignment.  */
> offset = ROUND_DOWN (offset, 8);
> --cut here--

Not convinced, having offset == 12 after the first line doesn't make sense.

I'd just beef up the comment:

/* Bump and round down to double word in case we already bumped by 4.  */
offset = ROUND_DOWN (offset + 8, 8);
diff mbox

Patch

Index: config/sparc/sparc.c
===================================================================
--- config/sparc/sparc.c	(revision 228726)
+++ config/sparc/sparc.c	(working copy)
@@ -4981,11 +4981,11 @@  sparc_compute_frame_size (HOST_WIDE_INT size, int
   else
     {
       /* We subtract STARTING_FRAME_OFFSET, remember it's negative.  */
-      apparent_frame_size = (size - STARTING_FRAME_OFFSET + 7) & -8;
+      apparent_frame_size = ROUND_UP (size - STARTING_FRAME_OFFSET, 8);
       apparent_frame_size += n_global_fp_regs * 4;
 
       /* We need to add the size of the outgoing argument area.  */
-      frame_size = apparent_frame_size + ((args_size + 7) & -8);
+      frame_size = apparent_frame_size + ROUND_UP (args_size, 8);
 
       /* And that of the register window save area.  */
       frame_size += FIRST_PARM_OFFSET (cfun->decl);
@@ -5116,7 +5116,7 @@  sparc_emit_probe_stack_range (HOST_WIDE_INT first,
 
       /* Step 1: round SIZE to the previous multiple of the interval.  */
 
-      rounded_size = size & -PROBE_INTERVAL;
+      rounded_size = ROUND_DOWN (size, PROBE_INTERVAL);
       emit_move_insn (g4, GEN_INT (rounded_size));
 
 
@@ -5317,7 +5317,7 @@  emit_save_or_restore_regs (unsigned int low, unsig
 	    emit_move_insn (gen_rtx_REG (mode, regno), mem);
 
 	  /* Always preserve double-word alignment.  */
-	  offset = (offset + 8) & -8;
+	  offset = ROUND_UP (offset, 8);
 	}
     }
 
@@ -6439,8 +6439,8 @@  function_arg_record_value_1 (const_tree type, HOST
 		  unsigned int startbit, endbit;
 		  int intslots, this_slotno;
 
-		  startbit = parms->intoffset & -BITS_PER_WORD;
-		  endbit   = (bitpos + BITS_PER_WORD - 1) & -BITS_PER_WORD;
+		  startbit = ROUND_DOWN (parms->intoffset, BITS_PER_WORD);
+		  endbit   = ROUND_UP (bitpos, BITS_PER_WORD);
 
 		  intslots = (endbit - startbit) / BITS_PER_WORD;
 		  this_slotno = parms->slotno + parms->intoffset
@@ -6495,8 +6495,8 @@  function_arg_record_value_3 (HOST_WIDE_INT bitpos,
   intoffset = parms->intoffset;
   parms->intoffset = -1;
 
-  startbit = intoffset & -BITS_PER_WORD;
-  endbit = (bitpos + BITS_PER_WORD - 1) & -BITS_PER_WORD;
+  startbit = ROUND_DOWN (intoffset, BITS_PER_WORD);
+  endbit = ROUND_UP (bitpos, BITS_PER_WORD);
   intslots = (endbit - startbit) / BITS_PER_WORD;
   this_slotno = parms->slotno + intoffset / BITS_PER_WORD;
 
@@ -6669,8 +6669,8 @@  function_arg_record_value (const_tree type, machin
       unsigned int startbit, endbit;
       int intslots, this_slotno;
 
-      startbit = parms.intoffset & -BITS_PER_WORD;
-      endbit = (typesize*BITS_PER_UNIT + BITS_PER_WORD - 1) & -BITS_PER_WORD;
+      startbit = ROUND_DOWN (parms.intoffset, BITS_PER_WORD);
+      endbit = ROUND_UP (typesize*BITS_PER_UNIT, BITS_PER_WORD);
       intslots = (endbit - startbit) / BITS_PER_WORD;
       this_slotno = slotno + parms.intoffset / BITS_PER_WORD;
 
@@ -7451,7 +7451,7 @@  sparc_gimplify_va_arg (tree valist, tree type, gim
     {
       indirect = false;
       size = int_size_in_bytes (type);
-      rsize = (size + UNITS_PER_WORD - 1) & -UNITS_PER_WORD;
+      rsize = ROUND_UP (size, UNITS_PER_WORD);
       align = 0;
 
       if (TARGET_ARCH64)
Index: config/sparc/sparc.h
===================================================================
--- config/sparc/sparc.h	(revision 228726)
+++ config/sparc/sparc.h	(working copy)
@@ -510,8 +510,7 @@  extern enum cmodel sparc_cmodel;
 #define SPARC_STACK_BOUNDARY_HACK (TARGET_ARCH64 && TARGET_STACK_BIAS)
 
 /* ALIGN FRAMES on double word boundaries */
-#define SPARC_STACK_ALIGN(LOC) \
-  (TARGET_ARCH64 ? (((LOC)+15) & ~15) : (((LOC)+7) & ~7))
+#define SPARC_STACK_ALIGN(LOC) ROUND_UP ((LOC), UNITS_PER_WORD * 2)
 
 /* Allocation boundary (in *bits*) for the code of a function.  */
 #define FUNCTION_BOUNDARY 32