Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
I think this is a backwards canonicalization decision and fabs + fcmp will be more broadly understood
llvm/test/Transforms/InstCombine/create-class-from-logic-fcmp.ll | ||
---|---|---|
1160 | This improvement will be picked up whenever my patch to combine class+fcmp lands |
Do you think this peephole should exist in backend? For targets with hardware instruction supporting is.fpclass, the intrinsic is more efficient than fabs+fcmp. Or, after __builtin_isinf_sign producing is.fpclass from frontend, this will not be needed?
Yes, the backend should handle optimally lowering fcmp and is.fpclass depending on instruction availability
I think this is better done in the backend. D143198 is the opposite direction but we could use both paths in the backend
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp | ||
---|---|---|
4969 ↗ | (On Diff #534885) | Is the advantage that you avoid materializing the inf constant? Can you check if the constantfp is legal? |
4972 ↗ | (On Diff #534885) | Could also handle the unordered case |
4976 ↗ | (On Diff #534885) | getTargetConstant |
llvm/test/CodeGen/AMDGPU/fp-classify.ll | ||
616 ↗ | (On Diff #534885) | The result is the same but I’d somewhat prefer to keep the original isa. The infinity constant is more likely reusable with other uses in the program |
llvm/test/CodeGen/AMDGPU/fp-classify.ll | ||
---|---|---|
616 ↗ | (On Diff #534885) | Fpclass checks are be better if the constant is used one or more times only in such comparison, I think? |
llvm/test/CodeGen/AMDGPU/fp-classify.ll | ||
---|---|---|
616 ↗ | (On Diff #534885) | These have identical code size and cycles. I think an infinity constant is more likely to appear from unrelated function uses. Plus fcmp is a better canonical form. You should avoid this if you base on only doing the transform if the constant is illegal |
Should check if the constant is legal. Also handling the unordered case isn't much more work
This improvement will be picked up whenever my patch to combine class+fcmp lands