[PR target/20126, RFC] loop DEST_ADDR biv replacement may fail

[Roger Sayle]

Hi Alex,

On 8 Apr 2005, Alexandre Oliva wrote:
> Roger suggested some changes in the patch.  I've finally completed
> bootstrap and test with and without the patch on amd64-linux-gnu, and
> posted the results to the test-results list.  No regressions.  Ok to
> install?

Hmm.  It looks like you misunderstood some of the comments in my
review (comment #16 in the bugzilla PR)...

+  gcc_assert (validate_change_maybe_volatile (v->insn, v->location,
+					       reg));

This is still unsafe.  If you look in system.h, you'll see that when
ENABLE_ASSERT_CHECKING is undefined, the gcc_assert macro gets defined
as:

#define gcc_assert(EXPR) ((void)(0 && (EXPR)))

which means that EXPR will not get executed.  Hence you can't put
side-effecting statements (especially those whose changes you depend
upon) naked inside a gcc_assert.  Ahh, I now see the misunderstanding;
you changed/fixed the other "safe" gcc_assert statement, and missed
the important one that I was worried about.  Sorry for the confusion.


Secondly:

+  if (volatile_ok
+      /* Make sure we're not adding or removing volatile MEMs.  */
+      || for_each_rtx (loc, volatile_mem_p, 0)
+      || for_each_rtx (&new, volatile_mem_p, 0)
+      || ! insn_invalid_p (object))
+    return 0;

The suggestion wasn't just to reorder the existing for_each_rtx to
move these tests earlier, it was to confirm that the original "whole"
instruction had a volatile memory reference in it, i.e. that this is
a problematic case, before doing any more work.  Something like:

+  if (volatile_ok
++     /* If there isn't a volatile MEM, there's nothing we can do.  */
++     || !for_each_rtx (&object, volatile_mem_p, 0)
+!     /* But make sure we're not adding or removing volatile MEMs.  */
+      || for_each_rtx (loc, volatile_mem_p, 0)
+      || for_each_rtx (&new, volatile_mem_p, 0)
+      || ! insn_invalid_p (object))
+    return 0;

This second change was just a micro-optimization, and I'd have approved
your patch without it, but the use of gcc_assert in loop_givs_rescan is
a real correctness issue.

Sorry again for the inconvenience,

Roger
--