Skip to content

Conversation

dougalm
Copy link
Collaborator

@dougalm dougalm commented Jul 11, 2023

Next we'll actually make decls part of it. But that will involve substantive changes. This first step just makes the type distinct from Binder so we can make all the administrative changes while still passing tests.

…nder`.

Next we'll actually make decls part of it. But that will involve substantive
changes. This first step just makes the type distinct from `Binder` so we can
make all the administrative changes while still passing tests.
Comment on lines +259 to -275
-- XXX: this is wrong. We need to add a mechanism for specifying commutativity
isAdditionMonoid :: BaseMonoid SimpIR n -> Maybe ()
isAdditionMonoid monoid = do
BaseMonoid { baseEmpty = (Con (Lit l))
, baseCombine = BinaryLamExpr (b1:>_) (b2:>_) body } <- Just monoid
, baseCombine = BinaryLamExpr _ _ _ } <- Just monoid
unless (_isZeroLit l) Nothing
PrimOp (BinOp op (Var b1') (Var b2')) <- exprBlock body
unless (op `elem` [P.IAdd, P.FAdd]) Nothing
case (binderName b1, atomVarName b1', binderName b2, atomVarName b2') of
-- Checking the raw names here because (i) I don't know how to convince the
-- name system to let me check the well-typed names (which is because b2
-- might shadow b1), and (ii) there are so few patterns that I can just
-- enumerate them.
(UnsafeMakeName n1, UnsafeMakeName n1', UnsafeMakeName n2, UnsafeMakeName n2') -> do
when (n1 == n2) Nothing
unless ((n1 == n1' && n2 == n2') || (n1 == n2' && n2 == n1')) Nothing
Just ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's true, but is there actually a reason to delete it? Without a check like this, the vectorizer will cheerfully reorder operations in arbitrary monoids, which may in principle emit incorrect code from a correct program.

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