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: x86 patch: SSE-based FP<=>int conversions, round 3



On Dec 17, 2006, at 5:39 PM, Jan Hubicka wrote:


Apologies for my tardy response; I've been having trouble with my
email client, and I didn't see your response until today.  :-P
Hi,
my apologizes for late reponse! I somehow forogt about your mail ;(

After all this work to "improve" FP conversions, it looks really dumb when somebody writes

double foo(int x) { return x; }

With the offered patch, GCC will do the conversion in an SSE register,
store that into the stack, and load the value into the x87; that's
three instructions. With -mfpmath=387, it all happens in one
instruction.


I think the patch can be made smarter, such that conversions hanging
from a return can be done in the x87, even when -mfpmath=sse.

(Apple chose -mfpmath=sse to deal with the x87 "excess precision"
issue, but there won't be any excess precision if we do one int => FP
conversion in the x87.)

Too bad that you didn't go for SSE passing convention too.

Not my decision.


I think without some hackery at cfgexpand side, all you can do is to
hide all the magic in one pattern and split it post-reload, that is
probably just too ugly to do.
I guess we are having hit or miss scenario here, at least for cases of
truncations that expand to non-trivial assembly.

Well, this isn't part of the offered patch, so for now it's moot.



+ "!TARGET_64BIT && SSE_FLOAT_MODE_P (<MODE>mode) && TARGET_SSE_MATH && TARGET_SSE2
+ && !optimize_size && (ix86_preferred_stack_boundary >= 128)"
+{
+ ix86_expand_convert_uns_<MODE>2SI_sse(operands); DONE;
+})
+
+;; Unsigned conversion to HImode.
+
+(define_insn "fixuns_truncdfhi2"
+ [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r")
+ (fix:HI (match_operand:DF 1 "nonimmediate_operand" "x,xm")))]
+ "TARGET_SSE2 && TARGET_SSE_MATH"
+ "cvttsd2si\t{%1, %k0|%k0, %1}"


You probably want {w} after the instruction template here so we are
consistent with using the operand size suffixes.

I tried this; Darwin's assembler doesn't like that. I guess this means that Darwin's assembler isn't really an AT&T assembler (?).


It looks to me like the Right Thing To Do is to create a third, non- AT&T, non-Intel/Microsoft assembler variant "ATT_Darwin."

Do you want a new assembler type added to my patch?

I am not sure right now, but won't it be better to simply convert
into
integer in this case?

(ie we can just dsable the x87 variant for conversion into short
and backend will automatically do (short)(int)float_var) that is
probably better than the prefixed operation anyway).
I believe this was my original plan too, somehow not happening.

Or is 16bit variant of the instruction faster on real hardware?

Without it, GCC will laboriously convert FP => unsigned_int32 using an
SSE conversion added in this patch (many instructions) and discard the
upper 16-bits to get a u_int16. It's much faster to convert FP
=> signed_int32 (one instruction), and then discard the upper 16 bits.


Is there a better way to accomplish this?

I personally would probably preffer to express different costs via
RTX_COST hook (ie that unsigned conversion is a lot more expensive) and
teach generic expander code to pick cheaper of the two alternatives.

O.K., we're talking about these two patterns I added into i386.md:


	fixuns_truncdfhi2
	fixuns_truncsfsh2

I can delete these from the patch without breaking correctness.

In the name of investigation, I disabled these patterns, rebuilt the compiler, and ran it under the debugger on this test case:

unsigned short int
df2uhi (double d)
{
  return d;
}

The conversion gets expanded in optabs.c:expand_fix(). expand_fix() asks can_fix_p() if the target can convert directly from DF to unsigned HI. This fails (the patterns mentioned above are gone), and expand_fix() loops around, trying SI as a wider mode. can_fix_p() answers yes, the target supports DF -> unsigned_SI; this is the fixuns_trunc<mode>si2 pattern added by my patch.

Having found a workable solution, expand_fix() immediately uses it. AFAICT, RTX_COST is not queried here.

Perhaps expand_fix() /should/ ask about RTX_COST ?

A separate breakpoint in ix86_rtx_costs() never sees the DF- >unsigned_SI conversion; it only sees RTX fragments from the insns generated by the fixuns_trunc<mode>si2 pattern.

I'm sorry to be dense, but I don't understand how to replace the fixuns_trunc[ds]fhi2 with RTX_COST. Clue appreciated.

O.K., so what would you like to see?  (A command-line switch telling
GCC it can rely on a 128-bit aligned stack?)

Here at Apple, we made a 128-bit stack the default, but we have a few
projects/situations where the stack gets misaligned. In those places,
one can specify -mpreferred-stack-boundary=2, and the check above will
prevent the SSE conversion from happening.

I am mostly concerned about targets where 128-bit alignment is not
maintained by runtime library (ie targets where GCC is not first class
citizen). I guess best way to handle with this is to add macro into
darwin and probably also linux/BSD config headers mandating that runtime
is 128-bit alignment friendly. When macro is not defined, disable the
trasnformations.

Done.


I agree that command line wise -mpreffered-stack-boundary=2 is probably
good enough to deal with special cases where you don't want aligned
stack.

Wow; I am astonished. O.K., happy to do it; shall I make this contingent on Intel targets?

(Are you sure? I'm very surprised that AMD prefers these values to go
through memory.)

It is in the official optimization manual. We even have
x86_inter_unit_moves hook for this, however it is disabled:
/* ??? Allowing interunit moves makes it all too easy for the compiler
to put integer data in xmm registers. Which results in pretty abysmal code. */


I don't think we need the broken flag. I would rather move the ???
comment down to ix86_secondary_memory_needed and disable it's use there,
set it for K8/Athlon and use it in your code.

O.K., in the initialization of x86_inter_unit_moves, I deleted the "0" and stripped the comments away from the "~(ATHLON_K8)", and I'm using the TARGET_INTER_UNIT_MOVES macro in the movdi_to_sse pattern. I respectfully prefer the ??? comment where it is.


With all due respect, why do you want the ??? comment to move?

+(define_insn_and_split "movdi_to_sse"
+  [(parallel
+    [(set (match_operand:V4SI 0 "register_operand" "=x")
+	  (subreg:V4SI (match_operand:DI 1 "register_operand"  "r") 0))

We don't want to use SUBREGs to access the scalars within vectors. We need to use instead the vec_merge stuff. See how loadld is implemented.

If your splitter trick is basically needed to deal with memory
operand,
why you don't allow "m" and don't have the easy path splitter here?

I'm sorry, I don't understand what you're suggesting. :-(


This splitter trick is to deal with DImode pseudos, and avoid copying
these values into the stack on their way to an %xmm register.

If we generate a simple SET to transfer a DImode value from a common
pseudo into an %xmm register, GCC will store the DImode pseudo
(usually %edx:%eax) into the stack and then load it into the %xmm.
This pattern shows GCC how to do this without the stack.

More importantly, because it has a delayed split, this pattern allows
the combiner to recognize

	load64			parm64 -> DImode.pseudo
	movedi_to_sse	DImode.pseudo -> %xmm

and turn it into

loadld parm64 -> %xmm

Of course, this is what we wanted in the first place, but right now
GCC is very reluctant to load a DImode directly into an %xmm.

Personally, I think that movdi_to_sse is a kludge.  The rest of the
patch will work without it, but the 32-bit-store/32-bit-store/64-bit-
load that results is almost as bad as doing it in the x87.

If you're suggesting something better that solves this problem, please
explain again. (Use simple words. :-)

I see, you are using two patterns, one matching the load, other matching
the register. This works for combine as you describe, but don't work
for register allocator - it is possible that register allocator will
chose to put the operand into memory because it is out of registers, but
it sees only the "r" constraint, so it will load it back for you.


If you make combined pattern for this, that allows both memory and
register operand and splits later for the case of register operand, it
will work better in this case.

Adding another constraint is trivial, so Done.


However, I still don't understand this. If the register allocator leaves the value in memory, then won't it be discovered there by one of the new int64->FP conversion routines? They already know how to load a DImode into an %xmm register.

It would be good to find a testcase to exercise this new constraint.

Most of the calls to ix86_expand_vector_move2() pass a constant source
operand that I want forced into memory; ix86_expand_vector_move() does
this, but it expects an array instead of two operands. This was
convenient.

If you do emit_move on the operands you pass to vector move, I would expected the ix86_expand_vector_move to be called and the operand offloaded to memory same way as you do. Or does replacing ix86_expand_vector_move2 calls fail for some reason?

You're right. Done.


Ian chose to clamp the input to a legal range so this conversion would
behave like the PPC. This is different from the x87, but I don't
think the behavior is defined when the input is out-of-range. (I
suppose we could omit the clamping under -ffast-math?)


Should I incorporate this code into the commentary?  I agree this
expander is incomprehensible; I'm open to suggestions.

Having the code in commentary would help at least me ;))

Done.


I wonder if the result is undefined, do we need the clamping with
-fno-fast-math at all?

My understanding of the C rules agrees with you; if the FP value exceeds the range of the integral result, the implementor can do anything.


I think we need an IEEE-754 lawyer here.


+ real_from_integer (&rvt_int_maxval, DFmode, 0xffffffffULL, 0ULL, 1);
+ int_maxval_as_fp = const_double_from_real_value (rvt_int_maxval, DFmode);
+
+ real_from_integer (&rvt_int_two31, DFmode, 0x80000000ULL, 0ULL, 1);
+ int_two31_as_fp = const_double_from_real_value (rvt_int_two31, DFmode);
+
+ incoming_value = force_reg (GET_MODE (operands[1]), operands[1]);


Similar tricks are played with ix86_build_signbit_mask and in SSE
conditional move expanders. It is probably desirable to commonize those
trick somewhat.

[snip]


Hm. (looks...) I agree, but this doesn't look trivial to me. If you
feel strongly about this, I can work on it, but I think it's equally
valid to keep my patch independent of these until it's accepted. (I
don't think my patch can use the current version of
ix86_build_signbit_mask(). If I have to rewrite
ix86_build_signbit_mask() as part of my patch, that makes my patch
even more complicated and hard to review, and I think my patch is
already too complicated.)

Yep, perhaps (especially because I can't approve this patch because I have rights for x86-64 only).

(Then why am I discussing this with... never mind. :-)


You might want to just copy the code form
one of the other two places so it looks identical instead of adding new
variant of it.

O.K., I split the vector-generation piece of ix86_build_signbit_mask out as a new function, and the new function replaces gen_2_4_rtvec. I also made an int => FP-const convenience function (ix86_fp_from_int_const) and that replaced the 'real_from_integer/ const_double_from_real' idiom above.


Bootstrap, DejaGnu on Darwin/x86_32, no regressions.
Bootstrap, DejaGnu on Darwin/x86_32 with -mfpmath=sse, in progress.

2006-12-21 Stuart Hastings <stuart@apple.com>

* gcc/testsuite/gcc.target/i386/20061023-1.c: New.
* gcc/config/i386/i386.md (fixuns_trunc<mode>si2, fixuns_truncdfhi2,
fixuns_truncsfhi2, fix_truncsfdi_sse, floatunssidf2, floatunsdidf3) New.
(floatdidf2): Calll ix86_expand_convert_sign_DI2DF_sse.
* gcc/config/i386/sse.md (movdi_to_sse): New.
* gcc/config/i386/i386-protos.h (ix86_expand_convert_uns_DF2SI_sse,
ix86_expand_convert_uns_SF2SI_sse, ix86_expand_convert_uns_DI2DF_sse,
ix86_expand_convert_uns_SI2DF_sse, ix86_expand_convert_sign_DI2DF_sse): New.
* gcc/config/i386/i386.h (TARGET_KEEPS_VECTOR_ALIGNED_STACK): New, defaults to false.
* gcc/config/i386/darwin.h (TARGET_KEEPS_VECTOR_ALIGNED_STACK): Enable for Darwin/x86.
* gcc/config/i386/i386.c (x86_inter_unit_moves): Change initialization.
(ix86_fp_from_int_const, ix86_expand_convert_uns_DF2SI_sse,
ix86_expand_convert_uns_SF2SI_sse, store_xmm_as_DF,
ix86_expand_convert_uns_DI2DF_sse, ix86_expand_convert_uns_SI2DF_sse,
ix86_expand_convert_sign_DI2DF_sse): New.
(ix86_build_const_vector): Extracted from ix86_build_signbit_mask.
(ix86_build_signbit_mask): Call it.




Attachment: gcc.fsf.cvt4.diffs.txt
Description: Text document


stuart hastings Apple Computer


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