Skip to content

fixup! [CIR][LowerToLLVM] Fix crash in PtrStrideOp lowering #947

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

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

smeenai
Copy link
Collaborator

@smeenai smeenai commented Oct 8, 2024

After recent rebases (likely as a result of [1]), the LLVMIR stride
operand can be generated by an unrealized_conversion_cast whereas the
CIR operand is coming directly from a block argument (and thus has no
defining op), so we need to separately check the CIR operand for the
existence of a defining op.

[1] llvm/llvm-project@2d50029

@smeenai
Copy link
Collaborator Author

smeenai commented Oct 8, 2024

This might be related to llvm/llvm-project#104668, which was reverted and relanded with a fix (with some further follow-ups going in). I'll make this a draft for now and come back to this during the next rebase.

@smeenai smeenai requested a review from lanza October 8, 2024 04:51
@smeenai smeenai marked this pull request as draft October 8, 2024 04:51
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Ready to move out of draft (and merge)!

@smeenai
Copy link
Collaborator Author

smeenai commented Oct 8, 2024

Ready to move out of draft (and merge)!

I'll see if this is still needed after the rebase I'm working on right now and revive it if needed :)

@ChuanqiXu9
Copy link
Member

I verified this is needed after the new trunk.

After recent rebases, the LLVMIR stride operand can be generated by an
unrealized_conversion_cast whereas the CIR operand is coming directly
from a block argument (and thus has no defining op), so we need to
separately check the CIR operand for the existence of a defining op.

Fixes llvm#912
@smeenai smeenai marked this pull request as ready for review October 11, 2024 17:25
@smeenai
Copy link
Collaborator Author

smeenai commented Oct 11, 2024

I spent some time trying to understand this more. If you go back to the original introduction of this commit in 9720c61, both the LLVMIR and CIR versions of the stride are coming directly from a block argument, so indexOp check fails. After the rebase, the LLVMIR version comes from an unrealized_conversion_cast but the CIR version still comes directly from the block argument, so the indexOp check succeeds and we run into the issue this commit is fixing.

I think that's related to llvm/llvm-project#101514. I'd misunderstood the follow-ups (such as llvm/llvm-project#107109 and llvm/llvm-project#106760) as removing unrealized_conversion_cast entirely (which got me confused about the need for this change), whereas they're actually related to removing it from the output IR but consistently having it present in the intermediate stages. I updated the description and I'll land it when CI passes.

@smeenai smeenai merged commit 62ec069 into llvm:main Oct 11, 2024
8 checks passed
@smeenai smeenai deleted the ptrstride-fix branch October 11, 2024 19:01
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