Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compiler: Patch CSE in presence of conditionals #2392

Merged
merged 1 commit into from
Jul 1, 2024
Merged

Conversation

FabioLuporini
Copy link
Contributor

No description provided.

@FabioLuporini FabioLuporini added the bug-C bug in the generated code label Jun 28, 2024
Copy link

codecov bot commented Jun 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.70%. Comparing base (7f77489) to head (90ad3e4).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2392   +/-   ##
=======================================
  Coverage   86.70%   86.70%           
=======================================
  Files         234      234           
  Lines       44409    44424   +15     
  Branches     8220     8219    -1     
=======================================
+ Hits        38503    38518   +15     
  Misses       5185     5185           
  Partials      721      721           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pe, changed = _uxreplace(pe, {k: v})
if changed and v not in scheduled:
updated.append(pe.func(v, k, operation=None))
scheduled.append(v)
updated.append(pe)
processed = updated
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use updated henceforth, rather than using processed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wdym ? we keep on looping and processed needs to be updated? what change are you proposing ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore me then

Copy link
Contributor

Choose a reason for hiding this comment

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

Missed an indent

@@ -168,6 +168,29 @@ def test_cse_temp_order():
assert type(args[2]) is CTemp


def test_cse_w_conditionals():
grid = Grid(shape=(10, 10, 10))
x, _, _ = grid.dimensions
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: x = grid.dimensions[0] would be tidier imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tidier but (nitpicking) the one above performs an implicit assertion

Copy link
Contributor

Choose a reason for hiding this comment

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

Fairs

Copy link
Contributor

@EdCaunt EdCaunt left a comment

Choose a reason for hiding this comment

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

Comments responded, looks good

@mloubout mloubout merged commit f67e7af into master Jul 1, 2024
31 checks passed
@mloubout mloubout deleted the fix-cond-placement branch July 1, 2024 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-C bug in the generated code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants