constified note_stores

constified note_stores

[Joern Rennecke]

I have been using note_stores to modify selected assignments.  Now when I
try to move this code to gcc 4.4, I find that I get a warning because
my walker function takes a non-const rtx - and if I make it take a const rtx,
there will be a warning somewhere inside because there is a code path
where a SET_SRC is modified.
data is in use to point to a libiberty hash table.

So, am I supposed to ignore the warning?
Roll my own copy of the original note_stores?
Resurrect the original note_stores in rtlanal.c (by whatever name)?

Re: constified note_stores

[Ian Lance Taylor]

Joern Rennecke <joernr@arc.com> writes:

> I have been using note_stores to modify selected assignments.  Now when I
> try to move this code to gcc 4.4, I find that I get a warning because
> my walker function takes a non-const rtx - and if I make it take a const rtx,
> there will be a warning somewhere inside because there is a code path
> where a SET_SRC is modified.
> data is in use to point to a libiberty hash table.
>
> So, am I supposed to ignore the warning?
> Roll my own copy of the original note_stores?
> Resurrect the original note_stores in rtlanal.c (by whatever name)?

Use CONST_CAST_RTX where necessary.

Ian

Re: constified note_stores

[Kaveh R. Ghazi]

From: "Ian Lance Taylor" <iant@google.com>

> Joern Rennecke <joernr@arc.com> writes:
>
>> I have been using note_stores to modify selected assignments.  Now when I
>> try to move this code to gcc 4.4, I find that I get a warning because
>> my walker function takes a non-const rtx - and if I make it take a const 
>> rtx,
>> there will be a warning somewhere inside because there is a code path
>> where a SET_SRC is modified.
>> data is in use to point to a libiberty hash table.
>>
>> So, am I supposed to ignore the warning?
>> Roll my own copy of the original note_stores?
>> Resurrect the original note_stores in rtlanal.c (by whatever name)?
>
> Use CONST_CAST_RTX where necessary.
> Ian

Or pass in a struct pointer to the "data" parameter containing both your 
hash table and the rtx to be modified.  Pull out either member in the walker 
function as necessary.

Me, I prefer being const-correct, but I understand not everyone shares my 
enthusiasm for it. :-/

        --Kaveh
--
Kaveh R. Ghazi

Re: constified note_stores

[Joern Rennecke]

> From: "Ian Lance Taylor" <iant@google.com>
> >Use CONST_CAST_RTX where necessary.

I think sprinkling const qualifiers liberally over the code and then
casting them away whenever they get in the way is worse than having
no const qualifiers in the first place.  It adds extra text to the code
which has no meaning, but might look to the casual observer as if it had.
And it makes the code more reliant on GNU extensions.

On Mon, Jun 09, 2008 at 04:45:14PM -0700, Kaveh R. Ghazi wrote:
> Or pass in a struct pointer to the "data" parameter containing both your 
> hash table and the rtx to be modified.  Pull out either member in the 
> walker function as necessary.

That would require to delare this ad-hoc struct, put all the code in to use
it, and use an asm to join the proper pointer value of the pointer to const
with the un-constified pointer to the rtx root.

Having another copy of note_stores seems simpler and is certainly
more portable.
What do you think about the name walk_stores?

Re: constified note_stores

[Ian Lance Taylor]

Joern Rennecke <joernr@arc.com> writes:

> Having another copy of note_stores seems simpler and is certainly
> more portable.
> What do you think about the name walk_stores?

Following this approach strictly leads to considerable code
duplication, which makes people unhappy.  If you want to make an
argument for just using it for note_stores, I'm willing to listen.  I
think the new name should be tied to note_stores, though--e.g.,
note_stores_nonconst.

It's definitely a problem in C that some functions reasonably take
const pointers in some cases and non-const pointers in other cases.
But I think it's a well understood problem, and I think
CONST_CAST_RTX, or small structures, are acceptable approaches.
Obviously if we had to use CONST_CAST_RTX everywhere something would
be wrong, but I only count six uses in the core code right now, which
seems like an acceptable compromise.

Ian

Re: constified note_stores

[Kaveh R. GHAZI]

On Tue, 10 Jun 2008, Joern Rennecke wrote:

> > From: "Ian Lance Taylor" <iant@google.com>
> > >Use CONST_CAST_RTX where necessary.
>
> On Mon, Jun 09, 2008 at 04:45:14PM -0700, Kaveh R. Ghazi wrote:
> > Or pass in a struct pointer to the "data" parameter containing both your
> > hash table and the rtx to be modified.  Pull out either member in the
> > walker function as necessary.
>
> That would require to delare this ad-hoc struct, put all the code in to
> use it, and use an asm to join the proper pointer value of the pointer
> to const with the un-constified pointer to the rtx root.

I don't understand the point about the asm.  And IMHO you overstate the
effort required to "put all the code in to use [the ad-hoc struct]".

One of the existing uses of CONST_CAST_RTX in alias.c also goes through
note_stores.  As an example, I wrote a quick (untested) patch to instead
use a struct with a const_rtx member.  It doesn't use an asm and seems to
be completely portable ISO C90.  I get no warnings when re-compiling
alias.o.

It's true, each function is one line longer and I had to declare the
struct.  But it is not an onerous amount of code.  I think you can do
something similar with a two-member struct containing your hash table.

		--Kaveh

diff -rup orig/egcc-SVN20080610/gcc/alias.c egcc-SVN20080610/gcc/alias.c
--- orig/egcc-SVN20080610/gcc/alias.c	2008-05-09 02:02:27.000000000 +0200
+++ egcc-SVN20080610/gcc/alias.c	2008-06-10 19:20:56.000000000 +0200
@@ -2361,15 +2361,24 @@ init_alias_target (void)
 #endif
 }

+/* This is a helper structure used to pass constant data through the
+   functions below.  */
+typedef struct mem_mod_data
+{
+  const_rtx mem;
+} mem_mod_data;
+
 /* Set MEMORY_MODIFIED when X modifies DATA (that is assumed
    to be memory reference.  */
 static bool memory_modified;
 static void
 memory_modified_1 (rtx x, const_rtx pat ATTRIBUTE_UNUSED, void *data)
 {
+  mem_mod_data *r = data;
+
   if (MEM_P (x))
     {
-      if (anti_dependence (x, (const_rtx)data) || output_dependence (x, (const_rtx)data))
+      if (anti_dependence (x, r->mem) || output_dependence (x, r->mem))
 	memory_modified = true;
     }
 }
@@ -2380,10 +2389,12 @@ memory_modified_1 (rtx x, const_rtx pat
 bool
 memory_modified_in_insn_p (const_rtx mem, const_rtx insn)
 {
+  mem_mod_data r = { mem };
+
   if (!INSN_P (insn))
     return false;
   memory_modified = false;
-  note_stores (PATTERN (insn), memory_modified_1, CONST_CAST_RTX(mem));
+  note_stores (PATTERN (insn), memory_modified_1, &r);
   return memory_modified;
 }

Re: constified note_stores

[Joern Rennecke]

On Tue, Jun 10, 2008 at 01:44:08PM -0400, Kaveh R. GHAZI wrote:
> I don't understand the point about the asm.

I need to modify the SET_SRC of the individual SETs presented by
note_stores.  The caller of note_stores can't pass in the SET RTXes,
since they vary for each call of the callback function; it can only pass
in a copy of the entire instruction pattern.
So this would be something like:

write_profile_sections (rtx dest ATTRIBUTE_UNUSED, const_rtx const_x,
			void *data)
{
  write_profile_sections_data *r = data;
  htab_t htab = r->htab;
  rtx x;

  __asm__ ("" : "=rX" (x) : "0" (const_x), "X" (r->pattern));
...

> And IMHO you overstate the
> effort required to "put all the code in to use [the ad-hoc struct]".

The current note_stores / walk_stores call is in an if clause, so adding
a variable declaration and two assignments also means that there needs
to be a new block statement.
note_stores isn't very large, and with the assembly cruft thrown in
and all this pretending to use const types when we aren't, the balance
is tilted in favour of another note_stores copy.

Re: constified note_stores

[Steven Bosscher]

On Tue, Jun 10, 2008 at 8:22 PM, Joern Rennecke <joernr@arc.com> wrote:
> On Tue, Jun 10, 2008 at 01:44:08PM -0400, Kaveh R. GHAZI wrote:
>> I don't understand the point about the asm.
>
> I need to modify the SET_SRC of the individual SETs presented by
> note_stores.  The caller of note_stores can't pass in the SET RTXes,
> since they vary for each call of the callback function; it can only pass
> in a copy of the entire instruction pattern.
> So this would be something like:
>
> write_profile_sections (rtx dest ATTRIBUTE_UNUSED, const_rtx const_x,
>                        void *data)
> {
>  write_profile_sections_data *r = data;
>  htab_t htab = r->htab;
>  rtx x;
>
>  __asm__ ("" : "=rX" (x) : "0" (const_x), "X" (r->pattern));
> ...

Eh, right, just the thought of doing this in GCC is IMVHO Wrong  ;-)

> note_stores isn't very large, and with the assembly cruft thrown in
> and all this pretending to use const types when we aren't, the balance
> is tilted in favour of another note_stores copy.

Actually, hold on a second please.  You're only talking in terms of
solutions.  But what *exactly* are you trying to do?  You ave to
modify the SET_SRC of some SETs.  What do these SET_SRCs look like?
This is important, because if you are only looking at registers, maybe
you can easily access them through DF_INSN_DEFS.

Gr.
Steven

Re: constified note_stores

[Joern Rennecke]

On Tue, Jun 10, 2008 at 08:27:43PM +0200, Steven Bosscher wrote:
> Actually, hold on a second please.  You're only talking in terms of
> solutions.  But what *exactly* are you trying to do?  You ave to
> modify the SET_SRC of some SETs.  What do these SET_SRCs look like?
> This is important, because if you are only looking at registers, maybe
> you can easily access them through DF_INSN_DEFS.

I am looking for special CONSTs that appear either as SET_SRC or as
memory addresses in SET_SRC or SET_DEST.

If I find a CONST with a specific UNSPEC there which is not hash table
yet, I emit some code; the CONST is then replaced with a SYMBOL_REF.

Or if you want the more abstract description, I'm shifting the burden of
profiling from the callee to the caller, thus saving some time and memory
for profiling, and getting acceptable profiling results when the libraries
contain only callgraph information instead of actual instrumentation -
i.e. we can do profiling without libg.a .