Re: Revision 144098 (d.d. Wed Feb 11 08:56:41 2009 UTC (4 weeks ago)) is not a regression bug fix.

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Wed, Mar 11, 2009 at 2:54 PM, Toon Moene <toon@moene.org> wrote:
> ... and it had to be "fixed" 3-4 times by H.J.Lu before it went so obscure
> to only hit people like me.
>
> I run a weather forecasting system 4 times daily to test it out.
>
> Because GCC would-be-4.4 is in regression fixes only during the last four
> months, I thought I could use it with impunity.
>
> Unfortunately, not so.
>
> From the 17th of February onwards, I find the following diagnostics in my
> output files:
>
> ?-------------------------------
> ?ABS(DPS)/3H = ? 1.007571 MB
> ?MAXWIND ? ? = ?70.835266 M/S IN (X,Y,LEV)= 129 145 ?15
> ?STPS ? ? ? ?= ? ? ? ? ? ?10077.023437500000
> ?STQ ? ? ? ? = ? ? ? ? ? ? ? ?4.921556472778
> ?STS ? ? ? ? = ? ? ? ? ? ? ? ?0.010799089447
> ?STPE ? ? ? ?= ? ? ? 2497269760.000000000000
> ?STKE ? ? ? ?= ? ? ? ? ?1622026.375000000000
> ?STTE ? ? ? ?= ? ? ? 2498891776.000000000000
>
> ?TOTAL RAIN RATE(mm/d)= ? ? ? ? ? ? ? ? ? ? +Infinity
> ?STRATIFORM PRECIP RATE= ? ? ? ? ? ? ? ? ? ? +Infinity
> ?CONVECTIVE PRECIP RATE= ? ? ? ? ? ? ? ?0.000000000000
> ?CWPATH = ? ? ? ? ? ? ? ? ? ? +Infinity
> ?COV2D = ? ? ? ? ? ? ? ? ? ? +Infinity
> ?HIGH CLOUDS = ? ? ? ? ? ? ? ? ? ? +Infinity
> ?MEDIUM CLOUDS = ? ? ? ? ? ? ? ? ? ? +Infinity
> ?LOW CLOUDS = ? ? ? ? ? ? ? ? ? ? +Infinity
> ?LOWEST LEVEL CLOUDS/FOG = ? ? ? ? ? ? ? ? ? ? +Infinity
> ?AVERAGED CLOUD BASE= ? ? ? ? ? ? ? ? ? ? +Infinity
> ?AVERAGED CLOUD TOP= ? ? ? ? ? ? ? ? ? ? +Infinity
> ?SENF = ? ? ? ? ? ? ? ? ? ? -Infinity
> ?LATF = ? ? ? ? ? ? ? ? ? ? ? ? ? NaN
> ?MOMF = ? ? ? ? ? ? ? ? ? ? +Infinity
>
> Without going into meteorological details, I hope you will trust me that the
> Infinity's, NaN's should not be there.
>
> After reverting r144098 and its updates today, the output looks normal:
>
> ?-------------------------------
> ?ABS(DPS)/3H = ? 1.144242 MB
> ?MAXWIND ? ? = ?75.913940 M/S IN (X,Y,LEV)= 155 151 ?13
> ?STPS ? ? ? ?= ? ? ? ? ? ?10076.395507812500
> ?STQ ? ? ? ? = ? ? ? ? ? ? ? ?4.889474391937
> ?STS ? ? ? ? = ? ? ? ? ? ? ? ?0.006965990644
> ?STPE ? ? ? ?= ? ? ? 2498901504.000000000000
> ?STKE ? ? ? ?= ? ? ? ? ?1634238.125000000000
> ?STTE ? ? ? ?= ? ? ? 2500535808.000000000000
>
> ?TOTAL RAIN RATE(mm/d)= ? ? ? ? ? ? ? ?0.150563895702
> ?STRATIFORM PRECIP RATE= ? ? ? ? ? ? ? ?0.146368727088
> ?CONVECTIVE PRECIP RATE= ? ? ? ? ? ? ? ?0.004195173737
> ?CWPATH = ? ? ? ? ? ? ? ?0.009085088037
> ?COV2D = ? ? ? ? ? ? ? ?0.115943998098
> ?HIGH CLOUDS = ? ? ? ? ? ? ? ?0.052225619555
> ?MEDIUM CLOUDS = ? ? ? ? ? ? ? ?0.031857494265
> ?LOW CLOUDS = ? ? ? ? ? ? ? ?0.062132436782
> ?LOWEST LEVEL CLOUDS/FOG = ? ? ? ? ? ? ? ?0.006665431429
> ?AVERAGED CLOUD BASE= ? ? ? ? ? ? ?448.916229248047
> ?AVERAGED CLOUD TOP= ? ? ? ? ? ? ?900.125244140625
> ?SENF = ? ? ? ? ? ? ?-30.753757476807
> ?LATF = ? ? ? ? ? ? ?-70.874824523926
> ?MOMF = ? ? ? ? ? ? ? ?0.177236527205
>
> So *during phase 4 - regression fixes only* a change *introducing* two
> peephole optimizations was not only proposed, but accepted and committed
> (and "fixed", at least three times).
>

Does this look correct?

(define_peephole2
  [(set (match_operand:SI 0 "register_operand" "")
        (match_operand:SI 1 "register_operand" ""))
   (parallel [(set (match_dup 0)
                   (match_operator:SI 3 "commutative_operator"
                     [(match_dup 0)
                      (match_operand:SI 2 "memory_operand" "")]))
              (clobber (reg:CC FLAGS_REG))])]
  "operands[0] != operands[1]
   && GENERAL_REGNO_P (REGNO (operands[0]))
   && GENERAL_REGNO_P (REGNO (operands[1]))"
  [(set (match_dup 0) (match_dup 4))
   (parallel [(set (match_dup 0)
                   (match_op_dup 3 [(match_dup 0) (match_dup 1)]))
              (clobber (reg:CC FLAGS_REG))])]
  "operands[4] = replace_rtx (operands[2], operands[0], operands[1]);")

If operands[2] uses operands[0], we replace it with operands[1]. It leads
to

(set (match_dup 0) (match_dup 4))

where (match_dup 4) uses operands[1] which may be uninitialized.
Shouldn't it be

 "operands[4] = replace_rtx (operands[2], operands[1], operands[0]);")

Did I miss something?


-- 
H.J.