refactor: replace is_fp32_dest_acc_en boolean with DestDatumWidth enum for type safety

[asr2003]

Ticket

Part of tenstorrent/tt-metal#24109

Problem description

Currently, the code uses a boolean template parameter is_dest_accum_en to control destination accumulation behavior.

What's changed

All boolean template parameter usage of is_dest_accum_en converted to enum class and parameter is renamed to fp32_dest_accumulation

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

Checklist

• All post commit CI passes
• Blackhole Post commit CI passes (if applicable)

[asr2003]

Hi @fvranicTT @nvelickovicTT

After migrating from the boolean is_fp32_dest_acc_en template parameter to the new DestAccumulation enum, I am unclear about the correct way to propagate this value at all call sites.

Previously we often passed a bool is_fp32_dest_acc_en with hardcoded true/false and recently migrated to usage of DST_ACCUM_MODE macro. Now since the template expects DestAccumulation::Enable or ::Disable what is the recommended way to convert an existing macro usage?

Should we always hardcode ::Enable/::Disable at call sites?

[fvranicTT]

Hi @fvranicTT @nvelickovicTT

After migrating from the boolean is_fp32_dest_acc_en template parameter to the new DestAccumulation enum, I am unclear about the correct way to propagate this value at all call sites.

Previously we often passed a bool is_fp32_dest_acc_en with hardcoded true/false and recently migrated to usage of DST_ACCUM_MODE macro. Now since the template expects DestAccumulation::Enable or ::Disable what is the recommended way to convert an existing macro usage?

Should we always hardcode ::Enable/::Disable at call sites?

We don't have this macro in this repository. Once this repo is clear of compile issues, we should try to integrate these changes into tt-metal. What I see that remains to be done here is replace boolean with new enum in the test cases.

[fvranicTT]

Hi @asr2003, could you please check contributing guidelines here? We're using pre-commit, which automatically fixes formatting issues, checks for spelling errors, etc? Thanks.

[fvranicTT]

After taking a deeper look into the changes, I see that enum class approach introduces a lot of boilerplate code needed for the macros to work. I have a bit different proposal, but I'd like to hear from @nvelickovicTT and @amahmudTT. It's a wrapper class. In short, something like this:

class DestAccumulation {
public:
  enum Value {
    Disable,
    Enable
  };

  DestAccumulation() = default;

  constexpr DestAccumulation(Value destAccumulation) : value(destAccumulation) { }

  constexpr bool operator==(DestAccumulation a) const { return value == a.value; }
  constexpr bool operator!=(DestAccumulation a) const { return !(*this == a); }

  // Boolean conversion
  constexpr operator bool() const { return value == Value::Enable; }

  // Optional: Access to raw enum
  constexpr Value raw() const { return value; }

private:
  Value value;
};

This way you could achieve the same thing as with enum class:

DestAccumulation d = DestAccumulation::Enable;

And also, thanks to boolean conversion, you can just pass dest accumulation object to TT/TTI macros.

[asr2003]

@fvranicTT This approach makes sense the way easier with current code. I have also thought of boolean conversion intially for tt-metal in caller sites. So thought to push PR into llk to validate here

[nvelickovicTT]

After taking a deeper look into the changes, I see that enum class approach introduces a lot of boilerplate code needed for the macros to work. I have a bit different proposal, but I'd like to hear from @nvelickovicTT and @amahmudTT. It's a wrapper class. In short, something like this:

class DestAccumulation {
public:
  enum Value {
    Disable,
    Enable
  };

  DestAccumulation() = default;

  constexpr DestAccumulation(Value destAccumulation) : value(destAccumulation) { }

  constexpr bool operator==(DestAccumulation a) const { return value == a.value; }
  constexpr bool operator!=(DestAccumulation a) const { return !(*this == a); }

  // Boolean conversion
  constexpr operator bool() const { return value == Value::Enable; }

  // Optional: Access to raw enum
  constexpr Value raw() const { return value; }

private:
  Value value;
};

This way you could achieve the same thing as with enum class:

DestAccumulation d = DestAccumulation::Enable;

And also, thanks to boolean conversion, you can just pass dest accumulation object to TT/TTI macros.

@fvranicTT Good point. Seems this would allow for a more condensed code (fp32_dest_acc instead of fp32_dest_acc == DestAccumulation::Enable), and still protect from accidental mistakes where someone uses int or boolean directly.

[sstanisicTT]

Maybe something like

class DestAccumulation {
public:
  static const DestAccumulation Enable;
  static const DestAccumulation Disable;

  constexpr bool operator==(DestAccumulation a) const { return value == a.value; }
  constexpr bool operator!=(DestAccumulation a) const { return !(*this == a); }

  constexpr operator bool() const {
    return value;
}

private:
  constexpr explicit DestAccumulation(bool value) : value(value) {}
  bool value;
};

// Definition of static members
constexpr inline DestAccumulation DestAccumulation::Enable{true};
constexpr inline DestAccumulation DestAccumulation::Disable{false};

Is more fitting:

• Doesn't allow wierd things like: DestAccumulation(DestAccumulation::Enable)
• Also if you do something like bool a = DestAccumulation::Enable with the other one it does Value -> int -> bool instead of Value -> DestAccumulation -> bool which isn't immediately obvious

[amahmudTT]

Please do two things,

1. For cases like (is_fp32_enable && some_other_cond), if you have changed it to something like ( fp_32_flag == fp32_flag::ENALBE && some_other_cond) , please put the first check in parenthesis.

2. For the git diff output , please grep for "=" and make sure none of the "==" mistakenly got replaced by a single "=" , as that would silently be ignored.

[github-actions]

✨ A mirror branch has been created/updated for this PR: mirror/asr2003/DST-enum

[fvranicTT]

@asr2003 could you take a look at the mirrored branch? I made some changes that circumvent the problem of C++ versions. Also, we've decided to rename the class to better reflect what it does. I've made changes for wormhole, but they might be incomplete/tests failing. I haven't touched BH code. Please take a look. Thank you and sorry to keep you waiting this long.

[marty1885]

Hello! @asr2003 any luck?

[asr2003]

Hi @marty1885. I have few errors to resolve once there are done, I will push latest changes here by today or tomorrow

[asr2003]

For pre-commit check, I will run it and push after a while(I am facing some installation issues of it)

[asr2003]

@fvranicTT I have fixed pre-commit check fail. Can I get latest status of CI?

[asr2003]

@fvranicTT I have ported tests now to use new class. Can I get latest status of CI?

[asr2003]

@nvelickovicTT @fvranicTT @amahmudTT @marty1885 Can I get latest status of CI?

[asr2003]

@nvelickovicTT @fvranicTT @amahmudTT @marty1885 Can I get latest status of CI?

[asr2003]

@marty1885 Can I get latest status of CI? I have fixed the format issues that caused last CI to fail

[marty1885]

@asr2003 Sorry I missed your messages - yeah the tests are failing. That needs to be fixed. Do you have hardware access? I don't think debugging on the CI is a viable strategy. I can give you could access. Would that help?

[marty1885]

Hello @asr2003 any updated? I'm obligated to check and I'll reopen the issue if you have lost interest on Firday