[llvm] r224458 - R600/SI: Fix f64 inline immediates

[Matt Arsenault]

> On Jan 3, 2015, at 3:05 PM, Daniel Sanders <Daniel.Sanders at imgtec.com> wrote:
> 
>> From: Matt Arsenault [whatmannerofburgeristhis at gmail.com] on behalf of Matt Arsenault [arsenm2 at gmail.com]
>> Sent: 02 January 2015 16:24
>> To: Daniel Sanders
>> Cc: Matthew P Arsenault; llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm] r224458 - R600/SI: Fix f64 inline immediates
>> 
>>> On Dec 23, 2014, at 3:09 PM, Daniel Sanders <Daniel.Sanders at imgtec.com> wrote:
>>> 
>>> Hi Matt,
>>> 
>>> This commit added some tests that fail on the Mips builder. I've had a quick look and I don't understand the failure at the moment. As far as I can tell, add_inline_imm_neg_2_f32 is being given 0xffffffffc0000000 (double precision -NaN) as the immediate input to the fadd. Somewhere along the lines, the -NaN becomes NaN on this Mips host, and it eventually prints 0x7fbfffff (single precision NaN) for the v_add_f32_e64 instruction. The test is expecting '-2' and works on my x86_64 host.
>>> 
>>> I don't understand the R600 targets instruction set so I might be missing something obvious. The main thing I don't understand is that the test provides -NaN as the input but is expected to emit -2 in the output. Could you explain?
>> 
>> Does Mips use a strange value for NaN? I believe in some places the tests for valid inline immediate constants end up using host floating point variables.
> 
> By modern standards, probably yes. Most Mips systems use IEEE754-1985 compliant NaN's but not IEEE754-2008 compliant ones. See below.
> 
>> I think the reason it uses -2 is because any value for NaN can be used as long as all of the exponent bits are 1, and the fraction is non-zero. Constants from -16 to 64 are free to use as operands usually, so picking a value that > > satisfies this should work. I’m not sure why exactly it wouldn’t use -1 though. I do not believe I changed how this was represented. I think what I did change was that values that used to be bitcasted to integers are now floating > point, so later when checking the value it ends up using a host float.
> 
> Ah yes, integer -2 is a bitpattern for a single precision NaN. I was thinking the instruction meant -2.0 for some reason.
> 
> I think I know what's going on and I agree that this commit didn't change the behaviour, it merely added a test that was affected by it.
> 
> The definition of NaN you gave above is correct but there are two kinds of NaN, signalling and quiet. IEEE754-1985 didn't specify exactly how they were distinguished but IEEE754-2008 specifies the most significant fraction bit to be an is_quiet flag (1=quiet, 0=signalling). Unfortunately, Mips originally chose a different definition (the same bit but opposite meanings) to the one that 2008 eventually prescribed and as a result we have ended up with two definitions controlled by a hardware config bit. Up until very recently (Mips32r6/Mips64r6), hardware support for 2008 was optional so most software uses the 1985 definition for compatibility reasons. APFloat uses the 2008 definition.
> 
> The root of the problem is that MCOperand uses the host double type to store target values. This isn't much of a problem by itself but there's also an implicit conversion from float to double in AMDGPUMCInstLower::lower() when it calls MCOperand::CreateFPImm() for the single precision case.

> QNaN promoted to double is still QNaN but there's no requirement to preserve any of the sign or fraction bits (except the is_quiet bit). On Mips, this conversion produces 0x7fbfffff which is a QNaN for most Mips systems, but an SNaN according to IEEE754-2008.  So the is_quiet bit flips because the host and target disagree on whether it's an is_quiet or an is_signalling bit and the value was promoted by the host. We lost the sign bit as well because nothing in the standard specifies that the sign bit has to be preserved (because it's meaningless for NaN).
> 
> Presumably the correct fix is to store APFloat's in MCOperand instead of host doubles. That's quite a big change though.

Tom is working on a patch which will remove all uses of fpimm operands and replace them with the bitcasted integers, which I think will fix this. I think this was actually broken by one of the previous patches in this set that stopped promoting some float operations to integers.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150106/0655c244/attachment.html>