Re: i386.md improvements III

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

  In message <19980929121229.26089@atrey.karlin.mff.cuni.cz>you write:
  > Hi
  > I am continuing in development at i386.md file. Now it seems to generate
  > slightly better code at all x86 clones, contains more exact scheduling for
  > Pentium and split some significant pattern to improve scheduling. I have
  > some other changes pending, but I want to receive some feedback about this
  > patch first, since I am still unsure if I am at right track (I think I
  > understand gcc much better now, but still I don't feel as expert :))
First I'd like to say thanks for the work and your patience.

I don't personally know anything about the Pentium pipelines, so I'd like
someone that does (John Wehle comes immediately to mind) to take a look at
your changes.  It would also be good if you and John could work together to
hash out conflicts since he's also got a patch which is supposed to improve
scheduling on Pentiums.

  > Hope that my submission of patch is formally correct now. I've also sent
  > assign.future, so hope to receive some feedback.
Good.  That was the next issue that needed to be addressed.  We can't actually
install any of your changes until the copyright assignment stuff is wrapped up.
We can speed up the process a little if you send a copy of the assignment to
me too.

  >  Then I plan to look more
  > closely at FP patterns and split some DI mode aritmetic. Also add MD_SCHED
  > macros for pentium pipelines.
Be very careful with the DImode patterns.  They've been the source of many
headaches over the years.

I see some gratuitous whitespace changes, which both make the patch larger
and violate GNU style guidelines.  Please remove them.

At this point I fear your patch is getting too big to be easily reviewed, so
I'd like to break it down into more managable chunks before you go any further.

  1. New define_splits

  2. New attributes on *existing* instructions

  3. all the rest


After we deal with #1 and #2, we'll decide if the remainder needs to be split
up any further.  But I would like you to first fix the various coding standards
problems I'll mention shortly, then submit just the new define_splits.    We'll
deal with that, then move onto the next group of changes.  Divide and conquer.



  > --- 370,375 ----
  > ***************
  > *** 243,249 ****
  >     ""
  >     "*
  >   {
  > !   if (REG_P (operands[0]))
  >       return AS2 (test%L0,%0,%0);
  >   
  >     operands[1] = const0_rtx;
  > --- 395,401 ----
  >     ""
  >     "*
  >   {
  > !   if (REG_P(operands[0]))
  >       return AS2 (test%L0,%0,%0);
  >   
  >     operands[1] = const0_rtx;
Here's an example of the gratuitous whitespace changes I mentioned above.  This
occurs in a few places in your patch.  I would recommend you look at each hunk
in your patch and make sure you aren't making uncalled for whitespace changes.


  >     /* For small integers, we may actually use testb. */
  >     if (GET_CODE (operands[1]) == CONST_INT
  >         && ! (GET_CODE (operands[0]) == MEM && MEM_VOLATILE_P (operands[0]))
  > !       && (! REG_P (operands[0]) || QI_REG_P (operands[0]))
  > !       /*At Pentium test is pairable only with eax. Not with ah or al*/
  > !       && (! REG_P (operands[0]) || REGNO(operands[0]) || ix86_cpu!=PROCESSOR_PENTIUM
  > !           || optimize_size))
You're missing lots of whitespace in here.  I would recommend you pick up a
copy of the GNU coding standards (available on our web site) and review them.

You need a space before the "At Pentium", and what does "At Pentium" mean?
Always put two spaces after a period and all comments should be complete
sentences.  My best guess is the comment should read

  > !       /* A Pentium test is pairable only with eax.  Not with ah or al.  */

The next line is missing whitespace in a few places.  I think it should read:
  > !       && (! REG_P (operands[0]) || REGNO (operands[0]) || ix86_cpu != PROCESSOR_PENTIUM


I recommend you go through each hunk and make sure your indention and
whitespace follow GNU coding standards.  You don't need to fix all the
problems in the x86 files, just those in code you change.



  > + ;; Split the trivial case of movsi
  > + (define_split 
  > +   [(set (match_operand:DI 0 "general_operand" "or")
  > + 	(match_operand:DI 1 "general_operand" "or"))]
  > +   "!reg_overlap_mentioned_p (operands[0], operands[1]) &&
  > +    (reload_completed | reload_in_progress)"
  > +   [(set (match_dup 3) (match_dup 5))
  > +    (set (match_dup 4) (match_dup 6))]
  > +   "split_di (&operands[0], 1, &operands[3], &operands[4]);
  > +    split_di (&operands[1], 1, &operands[5], &operands[6]);")
The "&&" in the condition above belongs on the next line.  Line breaks are
to be made before "&&", "||", etc not after them.  You made this mistake in
many places.  You need to fix them.

Use "||" instead of "|" above too -- you're testing if either of two booleans
is nonzero, not value of the bitwise-or of the two values.  You made this
mistake in a few places too.  While they're functionally equivalent, using
"||" more clearly states your intended purpose.


  > ! ;; When parameter is in memory, copy it to destination and then again
  > ! (define_split 
  > !   [(set (match_operand:DI 0 "general_operand" "r")
  > ! 	(sign_extend:DI (match_operand:SI 1 "general_operand" "mr")))]
  > !   "(reload_completed | reload_in_progress) &&
  > !    (!REG_P (operands[0]) || REGNO (operands[0]) != 0  ||
  > !    (!optimize_size && ix86_cpu==PROCESSOR_PENTIUM))"
  > !   [(set (match_dup 3) (match_dup 1))
  > !    (set (match_dup 4) (match_dup 3))
  > !    (set (match_dup 4)
  > !         (ashiftrt:SI (match_dup 4) (const_int 31)))
  > !   ]
The closing ']' belongs on the previous line.  Plus the usual whitespae and
line break problems.


  > --- 3784,3798 ----
  >   
  >   (define_insn "divqi3"
  >     [(set (match_operand:QI 0 "register_operand" "=a")
  > ! 	(truncate:QI (div:HI (match_operand:HI 1 "register_operand" "0")
  > ! 		             (sign_extend:HI (match_operand:QI 2 "nonimmediate_
  > operand" "qm")))))]
  >     ""
  >     "idiv%B0 %2")
  >   
  >   (define_insn "udivqi3"
  >     [(set (match_operand:QI 0 "register_operand" "=a")
  > ! 	(truncate:QI (udiv:HI (match_operand:HI 1 "register_operand" "0")
  > ! 		              (zero_extend:QI (match_operand:QI 2 "nonimmediate
  > _operand" "qm")))))]
  >     ""
  >     "div%B0 %2"
  >     [(set_attr "type" "idiv")])
I'm not sure if it is safe to put the truncation in those patterns.  Some 
parts of gcc expect certain well known patterns to have rigidly defined
structures.  This may break those assumptions.


I'm sure I'll see more nits when you submit just the define_splits and I can
look at the changes more carefully.

jeff