Re: [Patch] fix for loop optimizer (strength reduction)

[Jeffrey A Law <law at cygnus dot com>]

  In message <00e401bf6376$624bd190$2a0da8c0@fred.mainsoft.com>you write:
  > 
  > Please someone integrate, since I dont have write access.
It's not a matter of you having write access or not, but a matter of
getting your patches reviewed.  Even if you had write access you would
still need to get your patches reviewed before they could be installed
into the sources.

Unfortunately, patch review is a general bottleneck right now -- it's
an artifact of project growth.  The number of incoming patches keeps
rising; which is presumably a healthy thing.  The problem is finding
enough qualified people that are willing to help review patches.


  > Bug report and test case:
  > http://gcc.gnu.org/ml/gcc-bugs/2000-01/msg00526.html
Thanks.  One thing that can help with testcases -- if you can put them into
the form we use for our testsuite we can integrate them independently of the
patch which fixes the bug.

This generally means stripping out useless stuff -- we want to generally
have tests which do not depend on include files or anything like that.

For execution tests the standard is to have the test call abort () if the
test fails and exit (0) if the test passes.  The test should produce no
other output.  It is also important to keep in mind that tests will be
run on a variety of targets with wordsizes from 16 - 64 bits, big and
little endian, etc.

Anyway, I cleaned up the test and installed it.  execute/20000225-1.c if you
want to see how it was transformed into a standard test for our testsuite.


  > When a giv (global induction variable) is used before its initial
  > assignment is is not correct to allow replacement by the strength reduced
  > variable, I added the code to detect this condition in function record_giv.
  >
  > 
  > Previously, this condition was caught later in the case of for loops (or
  > when the biv is incremented before the giv) by the code in
  > check_final_value, I think that's why this problem went undetected so far.
  > 
  > ChangeLog
  > 
  > 2000-01-20   Jose Luu   (jluu@mainsoft.com)
  > 
  >     gcc/loop.c (record_giv): detect if the giv is used before its
  > assignment, assert not_replaceable in this case.
You patch has a few problems that will need to be addressed before it can
be used.

First the "trivial stuff" -- the formatting is all wrong.  GCC has strict
formatting conventions that must be followed; the basics are discussed in
the GNU coding standards document which is available on our website.
http://gcc.gnu.org/contribute.html

Specifics include placement of the "{}" for block scoping purposes and
indention of the code is wrong.    Comments have a standard form too --
they should be complete sentences that end with a period, two spaces
and a */ to close the comment.

The formatting of your tests in your if conditional is wrong too.  Never
put a || && or similar operator on the end of a line.  They belong at the
start of a line.

  if ((GET_CODE (p) == INSN
       || GET_CODE (p) == JUMP_INSN
       || GET_CODE (p) == CALL_INSN)
      && reg_mentioned_p (v->dest_reg, PATTERN (p))))

Would be a more correct way to write your test.  An even more correct way
would be

  if (GET_RTX_CLASS (GET_CODE (p)) == 'i'
      && reg_mentioned_p (v->dest_reg, PATTERN (p))

Your code is also rather expensive in that it scans much of the loop and
has to call reg_mentioned_p on the pattern for each insn it scans.  Ouch!
I would be rather surprised if there aren't some variables you can check
to determine the relative positions of the first use and the first
set of a potential GIV.

jeff