Fix check for (neg) zero for fpclass emulation #2151
Conversation
We should convert to integer, not bitcast to it. Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>
|
@LU-JOHN please take a look |
Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>
lib/SPIRV/SPIRVWriter.cpp
Outdated
| BM->addCmpInst(OpIEqual, ResTy, BitCastToInt, ZeroConst, BB); | ||
| if (FPClass & fcPosZero && FPClass & fcNegZero) | ||
| APInt ZeroInt = APInt::getZero(BitSize); | ||
| auto *ZeroConst = |
There was a problem hiding this comment.
mmm, actually, should also move them under if/else
Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>
| return BM->addCmpInst(OpIEqual, ResTy, BitCastToInt, ZeroConst, BB); | ||
| } | ||
| // Created 'negated' zero | ||
| ZeroInt.setSignBit(); |
There was a problem hiding this comment.
As I understood APInt uses 2's complement, which does not have negative zero. Are you sure this is giving the correct result?
There was a problem hiding this comment.
this line in the test added after setting signed bit seems about right
There was a problem hiding this comment.
Right, it works because you're not doing any (2's complement) operations on the APInt.
There was a problem hiding this comment.
Hi @svenvh
Thanks for the review.
Is the use of APInt agreeable in this scenario, or do we have to change the logic here?
Thanks
There was a problem hiding this comment.
This becomes a moot point once the suggestion from @LU-JOHN below has been applied.
| auto *TestIsZero = | ||
| BM->addCmpInst(OpIEqual, ResTy, BitCastToInt, ZeroConst, BB); | ||
| if (FPClass & fcPosZero && FPClass & fcNegZero) | ||
| if (FPClass & fcPosZero && FPClass & fcNegZero) { |
There was a problem hiding this comment.
Instead of generating:
if (val==+0 || val==-0)
we might get better performance with:
if ((val&0x7fff...)==0)
There was a problem hiding this comment.
We need both val == 0 and val == -0 results. So, it is worthwhile to generate both.
Thanks
There was a problem hiding this comment.
I agree with @LU-JOHN 's suggestion; that should cover both cases.
There was a problem hiding this comment.
I think I might be missing something here. How will val & 0x&7fffffff give result for TestIsPosZero(..) and TestIsNegZero(..)? Is the idea here to replace val == 0 with (val & 0xFFFFFFFFF == 0) and val == -0 with (val && 0xFFFFFFFF != 0) && (val && 0x7FFFFFFF == 0)?
Can @svenvh or @LU-JOHN, please clarify? Thanks
There was a problem hiding this comment.
+0.0 is represented by 0x0000.0000 (for floats)
-0.0 is represented by 0x8000.0000 (for floats)
Instead of doing two tests we can check for both by doing:
(val & 0x7FFF.FFFF)==0x0000.0000
|
Closing as this was superseded by #2154 |
|
Thanks @svenvh |
We should compare not only to zero integer, but also 'negated' zero.