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

Allow raising ifs with speculatively executable loads #443

Merged
merged 7 commits into from
Mar 9, 2025

Conversation

ivanradanov
Copy link
Collaborator

No description provided.

@ivanradanov ivanradanov requested review from wsmoses and Pangoraw March 8, 2025 14:43
return false;

llvm::errs() << "ACCESSMAP\n";
isl_map_dump(accessMap);
Copy link
Member

Choose a reason for hiding this comment

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

@ivanradanov if there is an inner affine.for would this still work or potential crash?

we definitely want to exclude any if conditions, but we do need to ensure we have all the index vars

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently the use of this function is limited to only this case:

`scope` -> affine.if {
             ...
`op` ->      affine.load

so no indexes will be gone. I guess I will add a dominance check to make sure we don't misuse it in the future

@wsmoses
Copy link
Member

wsmoses commented Mar 9, 2025

MEMREFSHAPE
{ [i0] }
MEMREFSHAPE
{ [i0] : i0 >= 0 and i0 <= 193 }
ARRAY
{ [i0] : i0 >= 0 and i0 <= 193 }
{ [dim] -> [o0] }
({ [dim] -> [(3 + dim)] })
ACCESSMAP
{ [dim] -> [o0] : o0 = 3 + dim }
DOMAIN
{ [i0] : i0 >= 0 and i0 <= 190 }
ACCESSED
{ [i0] : i0 >= 3 and i0 <= 193 }
INBOUNDS
1
src/external/isl/isl_ctx.c:305: isl_ctx not freed as some objects still reference it

@ivanradanov
Copy link
Collaborator Author

@wsmoses did you try the latest commit? that should have removed the debug prints and fixed the memory handling

@wsmoses
Copy link
Member

wsmoses commented Mar 9, 2025

yeah I did [just running the test that failed CI, though in debug]

@ivanradanov
Copy link
Collaborator Author

Test should be fixed now, checking how it works on the full code now.

llvm::any_of(*ifOp.getElseBlock(),
[](Operation &op) { return !mlir::isPure(&op); })) {
[ifOp](Operation &op) {
return !isSafeToSpeculativelyExecuteAtScope(ifOp, &op);
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little worried about correctness here.

Specifically if you have an if of an if.

We want to use the scope of the outermost if (assuming that the outer if recursively calls this utility)

Copy link
Member

@wsmoses wsmoses left a comment

Choose a reason for hiding this comment

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

lgtm modulo if of if concerns

@wsmoses wsmoses merged commit c48b2a7 into main Mar 9, 2025
9 checks passed
@wsmoses
Copy link
Member

wsmoses commented Mar 9, 2025

will let the if concerns be addressed post commit @ivanradanov

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