Skip to content

Conversation

@ravil-mobile
Copy link
Contributor

This patch introduces SpeficFP matcher for SelectionDAG nodes.

This includes:

Adding SpecificFP_match() in SDPatternMatch.h.
Adding test coverage in SelectionDAGPatternMatchTest.cpp.

Closes #165566

@ravil-mobile ravil-mobile requested a review from RKSimon November 11, 2025 01:48
@github-actions
Copy link

github-actions bot commented Nov 11, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

if (const auto *CFP = dyn_cast<ConstantFPSDNode>(V.getNode()))
if (CFP->isExactlyValue(Val))
return true;
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to add splat handling as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I added. Could you, please, have a look?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to use m_c_BinOp if you want to test for commutation - otherwise you need to swap the operand matchers (or you can add another test and do both!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

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

Fix unit tests

@ravil-mobile ravil-mobile force-pushed the ravil/sd-specific-fp branch 2 times, most recently from 1229219 to 0c9d220 Compare November 20, 2025 01:03
@ravil-mobile ravil-mobile requested a review from RKSimon November 20, 2025 01:04
@github-actions
Copy link

github-actions bot commented Nov 20, 2025

🐧 Linux x64 Test Results

  • 186428 tests passed
  • 4868 tests skipped

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (ConstantFPSDNode *C = isConstOrConstSplatFP(V, /*AllowUndefs=*/true))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, did get the question. I guess you forgot to link the code snippet

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I meant why would I use m_SpecificFP(SDValue) and not just m_Specific(SDValue)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh. Ok, I understood. Ok, I removed m_SpecificFP(SDValue)

Copy link
Collaborator

Choose a reason for hiding this comment

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

TBH I'd prefer to use a APFloat for matching instead of converting to/from double

@mshockwave thoughts?

Copy link
Member

@mshockwave mshockwave Nov 20, 2025

Choose a reason for hiding this comment

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

yeah we can provide a double version of m_SpecificFP (which you did) but for the matcher class I prefer using APFloat as well. SpecificInt_match also uses APInt.
(also, why std::optional<double>?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah we can provide a double version of m_SpecificFP (which you did) but for the matcher class I prefer using APFloat as well.

Done

(also, why std::optional?)

removed. That was an artifact from a previous impl.

@ravil-mobile ravil-mobile requested a review from RKSimon November 20, 2025 16:32
@ravil-mobile ravil-mobile force-pushed the ravil/sd-specific-fp branch 3 times, most recently from b1ea084 to b185199 Compare November 20, 2025 23:20
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.

[DAG] SDPatternMatch - add generic m_SpecificFP(double) / SpecificFP_match matcher

3 participants