------- Additional Comments From roger at eyesopen dot com 2005-04-08 17:03 ------- Subject: Re: [PR target/20126, RFC] loop DEST_ADDR biv replacement may fail
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 -- -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20126
