Skip to content

Fix yet two more bugs#103

Open
davschneller wants to merge 4 commits intomasterfrom
davschneller/more-bugs
Open

Fix yet two more bugs#103
davschneller wants to merge 4 commits intomasterfrom
davschneller/more-bugs

Conversation

@davschneller
Copy link
Contributor

@davschneller davschneller commented Nov 25, 2025

  • allow one-element sum accumulations (i.e. result <= Add() + term)
  • prevent overriding a global variable when action merging

@davschneller davschneller marked this pull request as ready for review December 6, 2025 05:37
run: |
cd ./tests/code-gen
for example in matmul minimal indices slicing; do
for example in matmul minimal indices slicing bugfixing; do
Copy link
Contributor

Choose a reason for hiding this comment

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

The term "bugfixing" does not really say anything. Could you find a better name for this, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, it's meant to have test cases that would've failed before—i.e. bugs that were fixed (hence also the PR number where they were fixed as comments). Not sure how to name it better; maybe fixed_bugs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since it is more about testing for earlier bugs, I would include something that clarifies this is testing for older bugs. Something like testing_old_bugs or a shorter alternative. I don't like what I suggested because it is too long, but you could choose something comprehensive.

def visit_Add(self, node):
variables = [self.visit(child) for child in node]
assert len(variables) > 1
assert len(variables) >= 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, could you please elaborate a bit on this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The computation also works for length 1 without adjustments (but not so for 0 I think, but I haven't tried it); so all we do here is make the assert more lenient for that exact case.

(it doesn't quite return the original tensor here; but will still generate a copy-scale-add operation—though that one should be, IIRC, optimized away or merged with other operations afterwards)

Copy link
Contributor

Choose a reason for hiding this comment

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

How about we verify that it does not work for 0 first? If it also works for 0, maybe you could remove the assert.

Copy link
Contributor

@vikaskurapati vikaskurapati left a comment

Choose a reason for hiding this comment

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

There is a merge conflict. I have clarification questions, otherwise LGTM

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.

2 participants