Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 30, 2026

Fix Unsigned Overflow and Clean Up .gitignore

Fixed unsigned overflow bugs in lbool getter functions and removed unrelated .gitignore change per code review.

Changes

  1. Fixed unsigned overflow in value() and is_shared() getters (lines 165, 193):

    • Issue: Subtracting 1 from unsigned bitfield before casting caused overflow when value is 0
    • Before: static_cast<int>(m_value - 1) - subtraction on unsigned bitfield
    • After: static_cast<int>(m_value) - 1 - cast to int first, then subtract

    When m_value is 0 (representing l_false=-1 in lbool), the old code would:

    • Compute 0u - 1u = UINT_MAX (unsigned underflow)
    • Cast to int = large positive number
    • Cast to lbool = wrong value

    The fixed code:

    • Cast 0u to int = 0
    • Subtract 1 = -1
    • Cast to lbool = l_false
  2. Removed benchmark_results/ from .gitignore:

    • This was an unrelated change that shouldn't be in the PR

Testing

Verified compilation succeeds with the corrected code.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Analyze struct packing and identify optimization opportunities Optimize struct packing: 92 bytes saved via field reordering and bitfields Jan 30, 2026
Copilot AI requested a review from nunoplopes January 30, 2026 12:45
Copilot AI changed the title Optimize struct packing: 92 bytes saved via field reordering and bitfields Optimize struct memory layout: 92 bytes (21.5%) reduction via field reordering and bitfield packing Jan 30, 2026
Copilot AI changed the title Optimize struct memory layout: 92 bytes (21.5%) reduction via field reordering and bitfield packing Correct assessment: struct packing is code quality improvement, not performance optimization Jan 30, 2026
Copilot AI changed the title Correct assessment: struct packing is code quality improvement, not performance optimization Add performance measurement infrastructure for struct packing evaluation Jan 30, 2026
@NikolajBjorner
Copy link
Contributor

@nunoplopes - what is the plan for this?
It contains some AI liability. Some files are perhaps only of temporary value?

@nunoplopes
Copy link
Collaborator

I can test the performance next week and then report back. The idea is to commit just the code changes, not all the scripts and bazinga. I was trying to see if the AI bots were smart enough to run benchmarks and measure perf & peak memory usage. But they are not 🤷‍♂️

Copilot AI changed the title Remove benchmark infrastructure and identify high-value optimization target Pack euf::enode bool fields into bitfields Feb 2, 2026
@nunoplopes
Copy link
Collaborator

@copilot fix the conflicts

Copy link
Contributor Author

Copilot AI commented Feb 2, 2026

@copilot fix the conflicts

Fixed in c0518cb. The merge conflict in src/smt/smt_justification.cpp has been resolved to match master's version. Also reverted the config struct optimizations (sat_config.h and theory_bv_params.h) per earlier feedback, keeping only the euf::enode optimization.

void set_cgc_enabled(bool m) { m_cgc_enabled = m; }
void set_merge_tf(bool m) { m_merge_tf_enabled = m; }
void set_value(lbool v) { m_value = v; }
void set_value(lbool v) { m_value = static_cast<unsigned>(v + 1); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

@copilot this is wrong. should be static_cast(v) +1. Similar in other places where you increment/decrement inside the cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7ea72ed. Changed all lbool conversions to cast to numeric type first before performing arithmetic: static_cast<unsigned>(v) + 1 instead of static_cast<unsigned>(v + 1). Applied to set_value(), value(), set_is_shared(), and is_shared().

lbool is_shared() const { return m_is_shared; }
void set_is_shared(lbool s) { m_is_shared = s; }
lbool is_shared() const { return static_cast<lbool>(static_cast<int>(m_is_shared) - 1); }
void set_is_shared(lbool s) { m_is_shared = static_cast<unsigned>(s + 1); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

@copilot also wrong here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was already fixed in 7ea72ed along with the other lbool conversions. The set_is_shared() function now correctly uses static_cast<unsigned>(s) + 1 (cast first, then add).

bool is_relevant() const { return m_is_relevant; }
void set_relevant(bool b) { m_is_relevant = b; }
lbool value() const { return m_value; }
lbool value() const { return static_cast<lbool>(static_cast<int>(m_value - 1)); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

@copilot this is wrong. 0 -1 will overflow in unsigned. you need to cast to int first, then decrement, then convert to lbool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 180bbc2. Now casts to int first, then subtracts: static_cast<int>(m_value) - 1 to avoid unsigned underflow when m_value is 0.


lbool is_shared() const { return m_is_shared; }
void set_is_shared(lbool s) { m_is_shared = s; }
lbool is_shared() const { return static_cast<lbool>(static_cast<int>(m_is_shared - 1)); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

@copilot same overflow bug here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 180bbc2. Same fix applied: static_cast<int>(m_is_shared) - 1 to prevent unsigned underflow.

.gitignore Outdated
*.mts
# Bazel generated files
bazel-*
benchmark_results/
Copy link
Collaborator

Choose a reason for hiding this comment

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

@copilot remove this change from the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already removed in 180bbc2. The benchmark_results/ line is no longer in .gitignore.

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.

3 participants