Hi, Segher Boessenkool <seg...@kernel.crashing.org> writes:
> On Wed, Dec 14, 2022 at 04:26:54PM +0800, Jiufu Guo wrote: >> Segher Boessenkool <seg...@kernel.crashing.org> writes: >> > On Mon, Aug 29, 2022 at 11:42:16AM +0800, Jiufu Guo wrote: >> >> li %r9,-1 >> >> rldicr %r9,%r9,0,0 >> >> cmpd %cr0,%r3,%r9 >> > >> > FWIW, I find the winnt assembler syntax very hard to read, and I doubt >> > I am the only one. >> Oh, sorry about that. I will avoid to add '-mregnames' to dump asm. :) >> BTW, what options are you used to dump asm code? > > The same as GCC outputs, and as I write assembler code as: bare numbers. > It is much easier to type, and very much easier to read. > > -mregnames is fine for output (and it is the default as well), but > problematic for input. Take for example > li r10,r10 > which translates to > li 10,10 > while what was probably wanted is to load the address of the global > symbol r10, which can be written as > li r10,(r10) > > I do understand that liking the bare numbers syntax is an acquired taste > of course. But less clutter is very useful. This goes hand in hand > with writing multiple asm statements per line, which allows you to group > things together nicely: > li 9,-1 ; rldicr 9,9,0,0 ; cmpd 3,9 > Great! Thanks for your helpful comments! >> > Maybe it is better to not return magic values anyway? So perhaps >> > >> > bool >> > can_be_done_as_compare_of_rotate (unsigned HOST_WIDE_INT c, int clz, int >> > *rot) >> > >> > (with *rot written if the return value is true). >> Thanks for your suggestion! >> It is checking if a constant can be rotated from/to a value which has >> only few tailing nonzero bits (all leading bits are zeros). >> >> So, I'm thinking to name the function as something like: >> can_be_rotated_to_lowbits. > > That is a good name yeah. > >> >> +bool >> >> +compare_rotate_immediate_p (unsigned HOST_WIDE_INT c) >> > >> > No _p please, this function is not a predicate (at least, the name does >> > not say what it tests). So a better name please. This matters even >> > more for extern functions (like this one) because the function >> > implementation is always farther away so you do not easily have all >> > interface details in mind. Good names help :-) >> Thanks! Name is always a matter. :) >> >> Maybe we can name this funciton as "can_be_rotated_as_compare_operand", >> or "is_constant_rotateable_for_compare", because this function checks >> "if a constant can be rotated to/from an immediate operand of >> cmpdi/cmpldi". > > Maybe just "constant_can_be_rotated_to_lowbits"? (If that is what the > function does). It doesn't clearly say that it allows negative numbers > as well, but that is a problem of the function itself already; maybe it > would be better to do signed and unsigned separately. It makes sense. I updated a new version patch. > >> >> +(define_code_iterator eqne [eq ne]) >> >> +(define_code_attr EQNE [(eq "EQ") (ne "NE")]) >> > >> > Just <CODE> or <CODE:eqne> should work? >> Great! Thanks for point out this! <eqne:CODE> works. >> > >> > Please fix these things. Almost there :-) >> >> I updated the patch as below. Bootstraping and regtesting is ongoing. >> Thanks again for your careful and insight review! > > Please send as new message (not as reply even), that is much easier to > handle. Thanks! Sure, I just submit a new patch version. https://gcc.gnu.org/pipermail/gcc-patches/2022-December/608765.html Thanks a lot for your review. BR, Jeff (Jiufu) > > > Segher
