unnecessary assert

unnecessary assert

[Mike Stump]

[-- Attachment #1: Type: text/plain, Size: 2350 bytes --]

I found an assert that trips on my port for trivial constructs, often.  I'd like to remove it, so that my port works better.  The assert was added because the case analysis he did was for when BLKmode MEM stores appeared in back when he wrote the patch (Hi Jakub).  He didn't analyze when dealing with a non-BLKmode.  I've been through the code, and it previously handled non-BLKmode when the size was <= HOST_BITS_PER_WIDE_INT, so, I don't worry about that aspect of it. Indeed, the very assert was originally directly above code that was not more than HOST_BITS_PER_WIDE_INT bits safe:


-  gcc_assert ((unsigned) width <= HOST_BITS_PER_WIDE_INT);
-  
   /* Finish filling in the store_info.  */
   store_info->next = insn_info->store_rec;
   insn_info->store_rec = store_info;
   store_info->mem = canon_rtx (mem);
   store_info->alias_set = spill_alias_set;
   store_info->mem_addr = get_addr (XEXP (mem, 0));
   store_info->cse_base = base;
-  store_info->positions_needed = lowpart_bitmask (width);
+  if (width > HOST_BITS_PER_WIDE_INT)
+    {
+      store_info->is_large = true;
+      store_info->positions_needed.large.count = 0;
+      store_info->positions_needed.large.bitmap = BITMAP_ALLOC (NULL);
+    }
+  else
+    {
+      store_info->is_large = false;
+      store_info->positions_needed.small_bitmask = lowpart_bitmask (width);
+    }

The new code in that patch added support for BLKmode with sizes > HOST_BITS_PER_WIDE_INT.  The assert was to protect the positions_needed, as it wasn't big enough to handle any data larger than HOST_BITS_PER_WIDE_INT.  The previous patch now supports larger data and mediates access to positions_needed based upon is_large, which is necessary.  

The only outstanding question is, is there any other aspect of the code that needs to now check is_large, that doesn't.  I've looked and did not find any other such code.

[ digging ]

Ah, the original assert was added in:

svn+ssh://gcc.gnu.org/svn/gcc/trunk@134199

and merely protected positions_needed, as I suspected.

Ok?

Ok for 2.8?


http://gcc.gnu.org/PR31150 is the PR when the assert was added, if you want to see it.  svn+ssh://gcc.gnu.org/svn/gcc/trunk@142892 is the change itself.

2013-01-09  Mike Stump  <mikestump@comcast.net>

	* dse.c (record_store): Remove unnecessary assert.


[-- Attachment #2: dse.diffs.txt --]
[-- Type: text/plain, Size: 574 bytes --]

2013-01-09  Mike Stump  <mikestump@comcast.net>

	* dse.c (record_store): Remove unnecessary assert.

Index: dse.c
===================================================================
--- dse.c	(revision 195067)
+++ dse.c	(working copy)
@@ -1495,10 +1495,7 @@ record_store (rtx body, bb_info_t bb_inf
   if (GET_MODE (mem) == BLKmode)
     width = MEM_SIZE (mem);
   else
-    {
-      width = GET_MODE_SIZE (GET_MODE (mem));
-      gcc_assert ((unsigned) width <= HOST_BITS_PER_WIDE_INT);
-    }
+    width = GET_MODE_SIZE (GET_MODE (mem));
 
   if (spill_alias_set)
     {

Re: unnecessary assert

[Jakub Jelinek]

On Wed, Jan 09, 2013 at 02:57:00PM -0800, Mike Stump wrote:
> Ok for 2.8?

Not to 2.8, but to 4.8 it is ok.

> http://gcc.gnu.org/PR31150 is the PR when the assert was added, if you want to see it.  svn+ssh://gcc.gnu.org/svn/gcc/trunk@142892 is the change itself.

That actually didn't add the assert, just moved it from lots lines later
(unconditional there) to the conditional spot here.

> 2013-01-09  Mike Stump  <mikestump@comcast.net>
> 
> 	* dse.c (record_store): Remove unnecessary assert.
> 

> 2013-01-09  Mike Stump  <mikestump@comcast.net>
> 
> 	* dse.c (record_store): Remove unnecessary assert.
> 
> --- dse.c	(revision 195067)
> +++ dse.c	(working copy)
> @@ -1495,10 +1495,7 @@ record_store (rtx body, bb_info_t bb_inf
>    if (GET_MODE (mem) == BLKmode)
>      width = MEM_SIZE (mem);
>    else
> -    {
> -      width = GET_MODE_SIZE (GET_MODE (mem));
> -      gcc_assert ((unsigned) width <= HOST_BITS_PER_WIDE_INT);
> -    }
> +    width = GET_MODE_SIZE (GET_MODE (mem));
>  
>    if (spill_alias_set)
>      {


	Jakub