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

Ensure strict affine expr checking #448

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Ensure strict affine expr checking #448

wants to merge 4 commits into from

Conversation

vimarsh6739
Copy link
Member

@vimarsh6739 vimarsh6739 commented Mar 10, 2025

Multiplication of 2 dimensions ((d1 + 40) * (d0)) was being treated as an affine.store / affine.load, which is obviously wrong.
I'm thinking of raising it in the fallback case to a memref.store / memref.load

Comment on lines +478 to +479
bool lhsConst = isa<AffineConstantExpr>(bOpExpr.getLHS());
bool rhsConst = isa<AffineConstantExpr>(bOpExpr.getRHS());
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit (though I don't think this is relevant for GB)

Suggested change
bool lhsConst = isa<AffineConstantExpr>(bOpExpr.getLHS());
bool rhsConst = isa<AffineConstantExpr>(bOpExpr.getRHS());
bool lhsConst = bOpExpr.getLHS().isSymbolicOrConstant();
bool rhsConst = bOpExpr.getRHS().isSymbolicOrConstant();

bool rhsConst = isa<AffineConstantExpr>(bOpExpr.getRHS());

// either LHS or RHS has to be a constant
if (!lhsConst && !rhsConst)
Copy link
Member

Choose a reason for hiding this comment

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

the logic here needs to be more complex to be correct

for example, will accidentally fail if you have, say (s1 + 1) * (s2 + 2), since the lhs/rhs aren't symbols / constants

in any case, we can revisit this PR post GB

@ivanradanov
Copy link
Collaborator

ivanradanov commented Mar 10, 2025

I thought there is already a isPureAffine or something like that function in AffineExpr, can we use that?

   /// Returns true if this is a pure affine expression, i.e., multiplication,                                                                                                                                                                                                   
   /// floordiv, ceildiv, and mod is only allowed w.r.t constants.                                                                                                                                                                                                               
   bool isPureAffine() const;                                                                                                                                                                                                                                                    

Not sure if it properly handles dim * dim etc...

@vimarsh6739
Copy link
Member Author

vimarsh6739 commented Mar 10, 2025

Hmm, the implementation here seems to check it correctly here - https://github.com/llvm/llvm-project/blob/c8ec8073aa5d8e87a15d101ded149de399518bc1/mlir/lib/IR/AffineExpr.cpp#L212 (I didn't notice this, so thanks for pointing to this!)

Although I believe we can skip the recursive check here because we are already building the expressions recursively here.

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.

4 participants