-
Notifications
You must be signed in to change notification settings - Fork 23
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
structmemfunclowering: fix crash when gep is optimized away #203
Conversation
llvm is removing getPointerElementType(), so maybe that's why we do the overkill cast there. At some point we started removing all the "easy" cases of getPointerElementType usage. |
What is the actual type that we are trying to cast? |
In the example from issue from cheerp-meta, it was |
Ah I see that it's a completely degenerate case. Can we just skip applying the pass in this case? |
I'll check, I also saw this commit which I believe was never merged, it might also solve it: |
or even assuming that the type is char* if it's not inferrable from a gep |
LLVM is smart enough to be able to fold a gep if the source and indices are constants. However, it was assumed that it would always emit a gep instruction, which would cause a crash when trying to cast it. Upon further inspection, I believe that the surrounding if statement was meant as a way of working around one of such folds. The new code should work for the general case, so I've removed it. This fixes: leaningtech/cheerp-meta#141
f3e1901
to
a1235f5
Compare
I've removed the use of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the original issue is fixed, close it saying that it's fixed in master
LLVM is smart enough to be able to remove a gep if the source and indexes are constants. However, it was assumed that it would always emit a gep instruction, which would cause a crash when trying to cast it.
Upon further inspection, I believe that this casting to a GEPOperator and then getting the result element type is overkill, and we could just simply use the type of the value.
This fixes: leaningtech/cheerp-meta#141