Bogus REG_EQUIV note generation [was RE: Deep CSE bug!]

[Dave Korn]

[  Alexandre, I Cc'd you as this refers to an old patch of yours from ages
ago; if I didn't mention this up here you might wonder why you've suddenly
been added to a discussion you have had no part in.  This is purely FYI; I'm
not calling on you to rewrite your code but would value your input.  ]


    Hello, gcc-hacker!

  Ok, time for a new subject line, because Roger has convinced me that the
problem in the RTL I showed earlier:

(insn 33 32 37 0 0x1002f510 (set (subreg:SI (reg:DI 80) 4)
        (reg/v:SI 75)) 22 {movsi} (nil)
    (expr_list:REG_EQUAL (const_double -1 [0xffffffff] 0 [0x0] 0 [0x0] 0
[0x0] 0 [0x0] 0 [0x0])
        (nil)))

is that the REG_EQUAL note simply is not valid, because REG_EQUAL is meant
to refer to the SET_SRC, and so it should simply be a (const_int 0) which is
the related subword of the const_double as judged by the SUBREG_BYTE of 4 in
the SET_DEST.

  I've been stepping through cc1 and found out that the problem is down to
what seems to me to be bogus architecture in emit_move_insn.  Let me
explain:

We invoke emit_move_insn at the top level with:

x = (reg/v:DI 73)
y = (const_double 19088743 [0x1234567] -559038737 [0xdeadbeef] 0 [0x0] 0
[0x0] 0 [0x0] 0 [0x0])

LEGITIMATE_CONSTANT_P fails, so we try calling force_const_mem which
allocates a const pool entry for the value and returns

(mem/u/f:DI (symbol_ref:SI ("*.LC0")) [2 S8 A32])

which becomes our new value for y in the top level emit_move_insn, with the
const_double relegated to the y_cst role.  Then validize_mem emits the RTL
to load the symbol_ref into pseudo 74, so we have

x = (reg/v:DI 73)
y = (mem/u/f:DI (reg/f:SI 74) [2 S8 A32])
y_cst = (const_double 19088743 [0x1234567] -559038737 [0xdeadbeef] 0 [0x0] 0
[0x0] 0 [0x0] 0 [0x0])

  So far, this is valid.  But now comes the problem: the end of
emit_move_insn looks like this:

-	3208	  last_insn = emit_move_insn_1 (x, y);
 	3209	
-	3210	  if (y_cst && GET_CODE (x) == REG)
-	3211	    set_unique_reg_note (last_insn, REG_EQUAL, y_cst);
 	3212	
-	3213	  return last_insn;
-	3214	}

  IOW, it's implicitly assuming that whatever is returned from
emit_move_insn_1 is a single rtx which is equivalent to the overall effect
of the set we're currently emitting.

  But of course when we call emit_move_insn_1 and we don't have any movdi
pattern, it emits a series of individual movsi insns to do the block move.
So what gets returned to the invoking emit_move_insn is just the last insn
in a sequence of two movsi sets:

(set:SI (subreg:SI (reg/v:DI 73) 0)
	(mem/u/f:SI (reg/f:SI 74) [2 S4 A32]))
(set:SI (subreg:SI (reg/v:DI 73) 4)
	(mem/u/f:SI (plus:SI (reg/f:SI 74)
        (const_int 4 [0x4])) [2 S4 A32]))

and in fact neither of them correspond to the overall top-level set and
they're both in different modes to the actual datum.

  Now IMO this is an architectural bug.  Either a) emit_move_insn should
know that what it gets back from emit_move_insn_1 might represent some kind
of decomposition of the set it's trying to emit, and should do the
appropriate decomposition on the constant before adding the REG_EQUAL note,
and indeed should try and process all the multiple insns it gets back from
emit_move_insn_1, or b) emit_move_insn shouldn't be generating the note at
all, but it should be the responsibility of emit_move_insn_1, which actually
has the knowledge about what it's doing and why.  I would of course favour
the second solution, which localizes the knowledge about decomposition of
large mode moves into one place, rather than the first which effectively
duplicates it elsewhere and makes a requirement to keep it in sync!  I at
first thought that whichever patch originally broke out emit_move_insn_1
into a separate function must have left those couple of lines behind in the
wrong place, but then I found that Alexandre added them in 2001:

http://gcc.gnu.org/ml/gcc-patches/2001-01/msg01655.html

in order to take advantage of some optimisation opportunities that were
otherwise being lost when constants were forced out to memory in the
constant pool.  I guess the full complications of doing this weren't
immediately obvious!

  I think the correctest answer would probably be for emit_move_insn to pass
down y_cst (the constant equivalent of the SET_SRC for this insn) to
emit_move_insn_1, which can then do the equivalent processing (decompose
into SImode subparts, frex) to the const as it does to generate the insns it
actually emits, and so can attach valid REG_EQUAL notes to each of the insns
in a multi-insn load/store sequence.  I haven't attempted to implement this
yet because I've only just got finished with the diagnosis and writing this
report, but I'll give it a go and get back to y'all when I find out how well
it works.


    cheers, 
      DaveK
-- 
Can't think of a witty .sigline today....