This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombine] Fold setcc equal to infinity into is.fpclass
AbandonedPublic

Authored by qiucf on Feb 21 2023, 8:26 PM.

Diff Detail

Event Timeline

qiucf created this revision.Feb 21 2023, 8:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 8:26 PM
qiucf requested review of this revision.Feb 21 2023, 8:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 8:26 PM

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

qiucf added a comment.Feb 22 2023, 6:39 PM

I think this is a backwards canonicalization decision and fabs + fcmp will be more broadly understood

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?

I think this is a backwards canonicalization decision and fabs + fcmp will be more broadly understood

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

arsenm requested changes to this revision.Jun 22 2023, 7:59 AM

I think this is better done in the backend. D143198 is the opposite direction but we could use both paths in the backend

This revision now requires changes to proceed.Jun 22 2023, 7:59 AM
qiucf updated this revision to Diff 534885.Jun 27 2023, 2:46 AM
qiucf retitled this revision from [InstCombine] Fold (fcmp_oeq (fabs x) +Inf) into is_fpclass(x, fcInf) to [DAGCombine] Fold SETCC_OEQ(x, Inf) into IS_FPCLASS.

Implement in backend.

arsenm added inline comments.Jun 27 2023, 2:58 AM
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

qiucf added inline comments.Jun 27 2023, 3:08 AM
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?

arsenm added inline comments.Jun 27 2023, 3:04 PM
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

arsenm requested changes to this revision.Jul 7 2023, 5:52 AM

Should check if the constant is legal. Also handling the unordered case isn't much more work

This revision now requires changes to proceed.Jul 7 2023, 5:52 AM
qiucf updated this revision to Diff 557434.Sep 28 2023, 12:57 AM
qiucf marked 3 inline comments as done.
qiucf retitled this revision from [DAGCombine] Fold SETCC_OEQ(x, Inf) into IS_FPCLASS to [DAGCombine] Fold setcc equal to infinity into is.fpclass.