This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: wide int patch #6: Replacement of hwi extraction from int-csts.

you know richi, i did not know who i was actually talking to. i said who is this richard beiner person and then i saw the email address.


On 10/18/2012 08:58 AM, Richard Biener wrote: 
On Thu, Oct 18, 2012 at 2:52 PM, Kenneth Zadeck
<zadeck@naturalbridge.com> wrote:
On 10/18/2012 06:22 AM, Richard Biener wrote: 
On Wed, Oct 17, 2012 at 11:47 PM, Kenneth Zadeck
<zadeck@naturalbridge.com> wrote:
Richi,

I apologize for the size of this patch, but it only does one very small
thing, it is just that it does it all over the middle end.

This patch introduces a new api for extracting a signed or unsigned hwi
from
an integer cst.  There had been an abi for doing this, but it has some
problems that were fixed here, and it was only used sometimes.

The new abi consists of 6 functions, three for testing if the constant in
the int cst will fit and three for actually pulling out the value.

The ones for testing are tree_fits_uhwi_p, tree_fits_shwi_p,
tree_fits_hwi_p.   The first two of these are unsigned and signed
versions,
and the second one takes a boolean parameter which is true if the value
is
positive.   This replaces the host_integerp which basically has the same
functionality of tree_fits_hwi_p.   The reason for changing this is that
there were about 400 calls to host_integerp and all but 4 of them took a
constant parameter. These names are chosen to be similar to the similar
functions in wide-int and are more mnemonic that the existing name since
they use the more readable u and s prefixes that a lot of other places
do.

On the accessing side, there is tree_to_uhwi, tree_to_shwi, and
tree_to_hwi.
The first two do checking when checking is enabled. The last does no
checking.

Just a quick note here - the changelog mentions tree_low_cst (as new
function!?) but not host_integerp.  You should probably get rid of both,
otherwise uses will creap back as people are familiar with them
(I'm not sure these changes for consistency are always good ...)
i will fix this.
these are bugs in the changelog, not the code. new changelog included. 

I don't like that tree_to_hwi does not do any checking.  In fact I don't
like that it _exists_, after all it has a return type which signedness
does not magically change.  Unchecked conversions should use
TREE_LOW_CST.

the idea is that when wide-int goes in, there is actually no
TREE_INT_CST_LOW.   The concept of low and high go out the door. the int-cst
will have an array in it that is big enough to hold the value.
so tree_to_hwi becomes a short hand for just accessing the lower element of
the array.

you could argue that you should say tree_fits_uhwi_p followed by
tree_to_uhwi (and the same for signed).   This is an easy fix.   it just
seemed a little redundant.

Well, if you want raw access to the lower element (when do you actually
want that, when not in wide-int.c/h?) ... you still need to care about the
signedness of the result.  And tree_fits_uhwi_p does not return the
same as tree_fits_shwi_p all the time.

I don't see any goodness in tree_to_hwi nor tree_fits_hwi really.  Because
if you just access the lower word then that still has a sign (either
HOST_WIDE_INT or unsigned HOST_WIDE_INT).  We should get rid
of those places - can you enumerate them?  I think you said it was just
a few callers with variable signedness argument.
Note that tree_fits_hwi_p does check. it just takes a parameter to say if it wants signed or unsigned checking (it is the old host_integerp, repackaged). You really do need this function as it is for the 4 or 5 places it is called. The parameter to it is typically, but not always, the sign of the type of the int cst being passed to it.

it is the tree_to_hwi that is unchecked. Most of the places are identified with comments. This survives the changelog. (i happen to be in the group of people that think changelogs are useless, and that we should do a better job of commenting the code.)

I do not know if this is sloppyness or not, but the signedness that is checked rarely matches the sign of the variable that the value is assigned. I found this quite frustrating when i was making the patch but this kind of thing is common in code where the original writer "knew what (s)he was doing." Unless you are doing comparisons or shifting, the signedness of target does not really make much difference.

if you want me to change the sequences of explicit checking and unchecked access to explicit checking followed by a checked access, then i am happy to do this.

kenny
Richard.

I should also point out that about 2/3 if this patch is going to die as the
rest of the wide int stuff goes in.   But i did not want to submit a patch
that only converted 1/3 of the cases.   The truth is that most of these
places where you are doing this conversion are just because the people were
too lazy to do the math at the full precision of the double int.

Thus, my 2 cents before I really look at the patch (which will likely
be next week only, so maybe you can do a followup with the above
suggestions).

Thanks,
Richard.

it is expected normally, the unchecked accesses follows an explicit
check,
or if there is no explicit check, then one of the checked ones follows.
This is always not the case for places that just want to look at the
bottom
bits of some large number, such as several places that compute alignment.
These just use the naked unchecked access.

There were a lot of places that either did no checking or did the
checking
inline.   This patch tracks down all of those places and they now use the
same abi.

There are three places where i found what appear to be bugs in the code.
These are places where the existing code did an unsigned check and then
followed it with a signed access (or visa versa). These places are marked
with comments that contain the string "THE NEXT LINE IS POSSIBLY WRONG".
With some guidance from the reviewer, i will fix these are remove the
unacceptable comments.

Aside from that, this patch is very very boring because it just makes
this
one transformation. Look for interesting stuff in tree.h.   Everything
else
is just forcing everywhere to use a single abi.

This patch could go on with a little work without the other 5 patches or
it
can wait for them.

kenny


Attachment: p6-2.clog
Description: Text document

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]