Re: Ping Patch: PR target/24879: SSE3 intrinsic bug

["H. J. Lu" <hjl at lucon dot org>]

On Tue, May 02, 2006 at 08:51:49PM -0700, Ian Lance Taylor wrote:
> "H. J. Lu" <hjl@lucon.org> writes:
> 
> > On Tue, May 02, 2006 at 02:49:13PM -0700, Ian Lance Taylor wrote:
> > > "H. J. Lu" <hjl@lucon.org> writes:
> > > 
> > > > On Mon, May 01, 2006 at 11:41:16PM -0700, Ian Lance Taylor wrote:
> > > 
> > > > > I don't understand why the definition of __builtin_ia32_monitor and
> > > > > __builtin_ia32_mwait change for 64-bit mode.  As I understand it, they
> > > > > always take two SImode values.  So it seems to me that the type should
> > > > > be the same for both, whether you are in 32-bit mode or 64-bit mode.
> > > > > 
> > > > 
> > > > The IA32 SDM isn't very clear for this. I have checked with our chip
> > > > people.  The 64bit version should be
> > > > 
> > > > 	mwait  %rax,%rcx
> > > > 	monitor %eax,%rcx,%rdx
> > > 
> > > Yes, I believe I understand that.  But my understanding is that
> > > despite the use of 64-bit registers for the final two arguments of
> > > each instruction, only the low order 32-bits are significant.
> > > Therefore, it seems to me that the builtin function (not the
> > > instruction; the builtin function) should always use a 32-bit
> > > parameter type, whether you are doing a 32-bit build or a 64-bit
> > > build.  That is the part of your patch that I am questioning.
> > 
> > My understand is the builtin function will match the instruction. The
> > intrinsic will make sure that the proper bits are used. That is how
> > x86 builtin function/intrinsic works.
> 
> The builtin function is the builtin function, and the MD file insn is
> the MD file insn.  While they are normally similar, they are logically
> distinct and there is no special relationship between them.  This is
> obvious if you think about builtin functions like __builtin_memcpy.
> The function causes certain insns to be emitted, but there is no
> connection closer than that.

Most, if not all, of __builtin_ia32_xxx for intrinsics map to
instructions directly and users are supposed to use intrinsics, not
builtins directly. I have no strong opinions on this. I can certainly
change __builtin_ia32_mwait and __builtin_ia32_monitor not to follow
it.

> 
> In this case, since only 32-bit values are valid, it seems to me that
> the builtin function should logically accept only 32-bit values in
> both 32-bit and 64-bit mode.  Since the 64-bit mode instruction
> requires a 64-bit register, the 32-bit value should be zero-extended
> into a 64-bit register before emitting the instruction.

Users are supposed to include <pmmintrin.h> and use _mm_monitor and
_mm_mwait:

static __inline void __attribute__((__always_inline__))
_mm_monitor (void const * __P, unsigned int __E, unsigned int __H)
{
  __builtin_ia32_monitor (__P, __E, __H);
}

static __inline void __attribute__((__always_inline__))
_mm_mwait (unsigned int __E, unsigned int __H)
{
  __builtin_ia32_mwait (__E, __H);
}




H.J.