Skip to content

Conversation

@vpykhtin
Copy link

@vpykhtin vpykhtin commented Nov 2, 2025

PSDB only.

@z1-cciauto
Copy link
Collaborator

@michaelselehov
Copy link

5% improvement in 450.md on MI350 (as expected)

@vpykhtin vpykhtin force-pushed the amd/dev/vpykhtin/fma64_cond_opt branch from 506f858 to 80498f5 Compare November 6, 2025 15:37
@z1-cciauto
Copy link
Collaborator

Copy link

@michaelselehov michaelselehov left a comment

Choose a reason for hiding this comment

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

LGTM. Conditional approve after QA performance testing.
See minor comments in the code.

if (TrueConst)
return false;

// Check if select and the add/sub are in same loop context.
Copy link

@michaelselehov michaelselehov Nov 7, 2025

Choose a reason for hiding this comment

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

Optional: Consider adding a comment explaining why we skip loop-invariant selects:

// Check if select and the add/sub are in same loop context.
// Even for loop-invariant select (which might get hoisted), we skip
// the optimization because it wouldn't provide benefit in the loop body
// (same 1 instruction, but worse register pressure: 2 vs 4+ registers).
if (MLI->getLoopFor(MI.getParent()) != MLI->getLoopFor(SelMI->getParent()))
  return false;

This will help future maintainers understand the reasoning.

if (!FalseConst || !FalseConst->Value.isExactlyValue(0.0))
return false;

// Check if TrueVal is not constant.

Choose a reason for hiding this comment

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

Consider updating the comment to reflect that constant support is deferred:

// Check if TrueVal is not constant.
// TODO: Support constants in a follow-up patch. Currently disabled due to
// codegen issues that need investigation.
if (TrueConst)
  return false;


// New register.
LIS->createAndComputeVirtRegInterval(CondValueReg);

Choose a reason for hiding this comment

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

Optional: Add validation after LiveIntervals updates to catch potential issues early:

// Validate LiveIntervals after revert transformation
assert(LIS->verify() && "LiveIntervals verification failed after FMA revert");

Note: Check if LIS has verify() or similar method. The -verify-machineinstrs tests should catch issues anyway, but an explicit assert helps with debugging.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants