Skip to content

Conversation

@MrSidims
Copy link
Contributor

@MrSidims MrSidims commented Mar 30, 2023

Optimized LLVM IR started to contain this intrinsic recently after InstCombine updates, like:
"Fold and/or of fcmp into class" ( 08f03887 )

There is no direct counterpart for it in SPIR-V, so testing is mapped on a sequence of SPIR-V instructions:

  1. test for both qnan and snan -> OpIsNan
  2. test for either of qnan and snan -> isquiet(V) ==> abs(V) >= (unsigned(Inf) | quiet_bit)
    issignaling(V) ==> isnan(V) && !isquiet(V)
  3. test for neg/pos inf -> OpIsInf + OpSignBitSet
  4. test for neg/pos normal -> OpIsNormal + OpSignBitSet
  5. test for neg/pos subnormal -> issubnormal(V) ==> unsigned(abs(V) - 1) < (all mantissa bits set)
  6. test for neg/pos zero -> compare bitcasted to int float with 0 + OpSignBitSet
  • add test

There is no direct counterpart for it in SPIR-V, so testing is mapped
on a sequence of SPIR-V instructions:
1. test for qnan and snan -> OpIsNan
2. test for neg/pos inf -> OpIsInf + OpSignBitSet
3. test for neg/pos normal -> OpIsNormal + OpSignBitSet
4. test for neg/pos subnormal -> not (OpIsNormal or OpIsNan) +
   OpSignBitSet
5. test for neg/pos zero -> OpFOrdEqual + OpSignBitSet

Signed-off-by: Sidorov, Dmitry <[email protected]>
Copy link
Contributor

@jcranmer-intel jcranmer-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I've been reviewing several of the upstream patches related here, I felt I should give some drive-by comments.

The "correct" lowering of is_fpclass is given by TargetLowering::expandIS_FPCLASS (https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp#L8019). Instcombine should be canonicalizing the tests that are equivalent to a single comparison into a single instruction, so checking for the really simple cases may not be all that beneficial.

The general strategy for the implementation should not to be to check with floating-point operations, but integer operations on bitcast-to-int. This avoids issues where floating-point operations may have underspecified issues (e.g., comparison to zero returning different results depending on denormals-are-zero setting).

Signed-off-by: Sidorov, Dmitry <[email protected]>
@MrSidims
Copy link
Contributor Author

MrSidims commented Apr 5, 2023

@jcranmer-intel thanks! I've applied the comments. Not QNan, SNan, PosSubnormal, NegSubnormal, PosZero and NegZero are checked after bitcast to int. Checks for normal and inf remained to be the same - relying on existing SPIR-V instructions.

Signed-off-by: Sidorov, Dmitry <[email protected]>
@MrSidims MrSidims marked this pull request as ready for review April 6, 2023 15:14
Signed-off-by: Sidorov, Dmitry <[email protected]>
@MrSidims
Copy link
Contributor Author

MrSidims commented Apr 6, 2023

@jcranmer-intel @asudarsa @svenvh @maksimsab @jgstarIntel please take a look

; RUN: spirv-val %t.spv
; RUN: llvm-spirv -r %t.spv -o %t.rev.bc

; Just check that reverse translation works
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a quick check to ensure that fpclass is not generated here?

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's being checked implicitly by the fact, that forward translation doesn't error out due to unknown intrinsic

ResultVec.emplace_back(
GetNegPosInstTest(TestIsInf, FPClass & fcNegInf));
}
if (FPClass & fcNormal) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a possibility that fcNormal bit is set and both fcPosNormal and fcNegNormal are not set? In such such, what is the expected behavior? Thanks

Copy link
Contributor Author

@MrSidims MrSidims Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fcNormal is (fcPosNormal | fcNegNormal) so it's impossible scenario
same for inf, subnormal etc

Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. Just a couple of minor questions/comments.

Thanks

Signed-off-by: Sidorov, Dmitry <[email protected]>
@MrSidims
Copy link
Contributor Author

@asudarsa may I ask to take another look? I've added 2 enhancements: added caching for sign test results to be reused => less IsSignBit instructions generated and added check for inverted test aka (is inf, is normal, is subnormal is zero) = (is not nan)

Signed-off-by: Sidorov, Dmitry <[email protected]>
@MrSidims
Copy link
Contributor Author

@jcranmer-intel may I ask you to revisit the PR?

Copy link
Contributor

@jcranmer-intel jcranmer-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, the tests could be a little more comprehensive, but without an update-test-checks-like script, it is going to be too painful to make such a test for the limited extra value it would bring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants