Re: Debugging CSE, on the right track?

Re: Debugging CSE, on the right track?

[Jeffrey A Law]

  In message < 199807080001.CAA03897@jolan.ppro >you write:
  > The bug occurs because the CSE thinks expressions are equivalent that are
  > not equivalent.  The exact order in which this happens is as follows:
Correct.


  > (insn 18 16 20 (set (reg:SI 24)
  >         (reg:SI 0 %eax)) -1 (nil)
  >     (insn_list:REG_RETVAL 12 (expr_list:REG_EQUAL (expr_list (symbol_ref:SI
  >  ("f"))
  >                 (expr_list (reg/v:SI 23)
  >                     (nil)))
  >             (nil))))
  > 
  > RESULT:
  > 
  > EQUIVALENCES:
  > 
  > * (reg:SI 24)
  > * (expr_list (symbol_ref:SI ("f"))
  > 		      (expr_list (reg/v:SI 23)
  > 			  (nil)))
This is one of the two critical insns to watch.

  > ------------------------------------
  > * (reg:SI 24)
  > * (reg/v:SI 21)
  > * (expr_list (symbol_ref:SI ("f"))
  > 		      (expr_list (reg/v:SI 23)	<== WRONG!!! (reg/v:SI 23)
  > 			  (nil)))		             was changed!
Correct.

The real problem at hand is reg23 is used in a libcall, but is also
used outside of the libcall.  That is not supposed to happen -- to
prevent this exact problem.

I believe fixing this is relatively straightforward (copy r23 into
a new pseudo just before each libcall block, then use the new pseudo
in the libcall block).   I was unable to get such a patch to work
properly though, so I took an alternate track to fix the problem.

  > Isn't what we really WANT after that last instruction, the following
  > equivalences?
I don't really follow your equivalency lists.  But what we want is for 
the EXPR_LISTs to be correct -- instead of referencing r23, they
should reference (const_int 1) in the first one and (const_int 2) in
the second one.

  > Or - what seems more logical to me - don't we want to store the quantity
  > value of an expr_list instead of the expr_list itself, when this expr_list
  > is part of another expr_list?  I mean, it doesn't make much sense to
  > store
  > 
  > * (expr_list (symbol_ref:SI ("f"))
  >                       (expr_list (const_int 1)
  >                           (nil)))
  > 
  > either; It would make more sense to store:
  > 
  > * (expr_list (symbol_ref:SI ("f"))
  >                       (qty_value (26)
  >                           (nil)))
The EXPR_LISTs should reference the inputs to the libcall; in this
case the inputs are (const_int 0) and (const_int 1). 


  > if `26' is the qty value assigned to (const_int 1).
  > In that case we don't have to care about replacing
  > (reg/v:SI 23) when its value changes: You store the
  > quantity value to begin with.
  > 
  > Am I making any sense?
A little.
jeff

Debugging CSE, on the right track?

[Carlo Wood]

Hiya all.

I'd like feedback about my investigations...
Mainly because I want to learn how the compiler works, I did continue
my investigation of test suite case 980505-1.c *without* adding Jeffs
patches: I wanted to investigate what is going wrong before his patch.

Here are the results of my analysis so far:

The bug occurs because the CSE thinks expressions are equivalent that are
not equivalent.  The exact order in which this happens is as follows:

-------------------------------------------------------------------------------
RTX:

(insn 9 6 12 (set (reg/v:SI 23)
        (const_int 1)) -1 (nil)
    (nil))

RESULT:

INSN 9 became:

(insn 9 6 12 (set (reg/v:SI 23)
        (const_int 1)) 54 {movsi+2} (nil)
    (expr_list:REG_EQUAL (const_int 1)
        (nil)))

EQUIVALENCES:

* (const_int 1)
* (reg/v:SI 23)
 
-------------------------------------------------------------------------------
RTX:

(insn 12 9 14 (set (mem:SI (pre_dec:SI (reg:SI 7 %esp)))
        (reg/v:SI 23)) -1 (nil)
    (insn_list:REG_LIBCALL 18 (nil)))

RESULT:

INSN 12 became:

(insn 12 9 14 (set (mem:SI (pre_dec:SI (reg:SI 7 %esp)))
        (const_int 1)) 50 {movsi-2} (nil)
    (insn_list:REG_LIBCALL 18 (nil)))

EQUIVALENCES: no changes

-------------------------------------------------------------------------------
RTX:

(call_insn/u 14 12 16 (set (reg:SI 0 %eax)
        (call (mem:QI (symbol_ref:SI ("f")))
            (const_int 4))) -1 (nil)
    (nil)
    (nil))

RESULT: no changes

-------------------------------------------------------------------------------
RTX:

(insn 16 14 18 (set (reg:SI 7 %esp)
        (plus:SI (reg:SI 7 %esp)
            (const_int 4))) -1 (nil)
    (nil))

RESULT:

INSN 16 became:

(insn 16 14 18 (set (reg:SI 7 %esp)
        (plus:SI (reg:SI 7 %esp)
            (const_int 4))) 143 {addsi3+1} (nil)
    (nil))

EQUIVALENCES:

* (plus:SI (reg:SI 7 %esp)		<-- This one is on its own, so not
		      (const_int 4))          really equivalent at all :/
------------------------------------
* (const_int 1)
* (reg/v:SI 23)

-------------------------------------------------------------------------------
RTX:

(insn 18 16 20 (set (reg:SI 24)
        (reg:SI 0 %eax)) -1 (nil)
    (insn_list:REG_RETVAL 12 (expr_list:REG_EQUAL (expr_list (symbol_ref:SI ("f"))
                (expr_list (reg/v:SI 23)
                    (nil)))
            (nil))))

RESULT:

EQUIVALENCES:

* (reg:SI 24)
* (expr_list (symbol_ref:SI ("f"))
		      (expr_list (reg/v:SI 23)
			  (nil)))
------------------------------------
* (plus:SI (reg:SI 7 %esp)
		      (const_int 4))
------------------------------------
* (const_int 1)
* (reg/v:SI 23)

-------------------------------------------------------------------------------
RTX:

(insn 20 18 23 (set (reg/v:SI 21)
        (reg:SI 24)) -1 (nil)
    (nil))

RESULT:

INSN 20 became:

(insn 20 18 23 (set (reg/v:SI 21)
        (reg:SI 24)) 54 {movsi+2} (nil)
    (nil))

EQUIVALENCES:

------------------------------------
* (reg:SI 24)
* (reg/v:SI 21)					<-- added
* (expr_list (symbol_ref:SI ("f"))
		      (expr_list (reg/v:SI 23)
			  (nil)))
------------------------------------
* (plus:SI (reg:SI 7 %esp)
		      (const_int 4))
------------------------------------
* (const_int 1)
* (reg/v:SI 23)

-------------------------------------------------------------------------------
RTX:

(insn 23 20 26 (set (reg/v:SI 23)
        (const_int 2)) -1 (nil)
    (nil))

RESULT:

INSN 23 became:

(insn 23 20 26 (set (reg/v:SI 23)
        (const_int 2)) 54 {movsi+2} (nil)
    (expr_list:REG_EQUAL (const_int 2)
        (nil)))

EQUIVALENCES:

* (const_int 2)
* (reg/v:SI 23)
------------------------------------
* (reg:SI 24)
* (reg/v:SI 21)
* (expr_list (symbol_ref:SI ("f"))
		      (expr_list (reg/v:SI 23)	<== WRONG!!! (reg/v:SI 23)
			  (nil)))		             was changed!
------------------------------------
* (plus:SI (reg:SI 7 %esp)
		      (const_int 4))
------------------------------------
* (const_int 1)


-------------------------------------------------------------------------------
Ok, now my question :)...

Isn't what we really WANT after that last instruction, the following
equivalences?

* (const_int 2)
* (reg/v:SI 23)
------------------------------------
* (reg:SI 24)
* (reg/v:SI 21)
* (expr_list (symbol_ref:SI ("f"))
		      (expr_list (const_int 1)
			  (nil)))
------------------------------------
* (plus:SI (reg:SI 7 %esp)
		      (const_int 4))
------------------------------------
* (const_int 1)

Or - what seems more logical to me - don't we want to store the quantity
value of an expr_list instead of the expr_list itself, when this expr_list
is part of another expr_list?  I mean, it doesn't make much sense to
store

* (expr_list (symbol_ref:SI ("f"))
                      (expr_list (const_int 1)
                          (nil)))

either; It would make more sense to store:

* (expr_list (symbol_ref:SI ("f"))
                      (qty_value (26)
                          (nil)))

if `26' is the qty value assigned to (const_int 1).
In that case we don't have to care about replacing
(reg/v:SI 23) when its value changes: You store the
quantity value to begin with.

Am I making any sense?

-- 
 Carlo Wood  <carlo@runaway.xs4all.nl>