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

Fix equal pointers comparison when there is a GEP with more than 2 operands #247

Closed
wants to merge 1 commit into from

Conversation

rvan-mee
Copy link
Contributor

adds a check to see if a GEP with more then 2 operands is done on a pointer or an array, if this is the case we can compare the pointer/array and its offset directly without dereferencing the pointers

else if ((lhsKind == REGULAR || lhsKind == SPLIT_REGULAR || lhsKind == RAW ||lhsKind == CONSTANT ||
(isGEP(lhs) && (cast<User>(lhs)->getNumOperands()==2 || getGEPContainerType(cast<User>(lhs))->isArrayTy() || getGEPContainerType(cast<User>(lhs))->isPointerTy()))) &&
(rhsKind == REGULAR || rhsKind == SPLIT_REGULAR || rhsKind == RAW ||rhsKind == CONSTANT ||
(isGEP(rhs) && (cast<User>(rhs)->getNumOperands()==2 || getGEPContainerType(cast<User>(rhs))->isArrayTy() || getGEPContainerType(cast<User>(rhs))->isPointerTy()))))
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure how the container type could ever be a pointer here. Also, please add a small lambda to check for this condition. The lambda can be used for both operands.

Copy link
Member

@yuri91 yuri91 Aug 15, 2024

Choose a reason for hiding this comment

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

right, we were thinking about including pointer type assuming that the utility function accepted geps with just one index too, but it does not

@rvan-mee rvan-mee force-pushed the fix-pointer-equal-comparison branch from 3dd1496 to c53613b Compare August 15, 2024 14:01
Copy link
Member

@yuri91 yuri91 left a comment

Choose a reason for hiding this comment

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

one test is failing in the CI

@rvan-mee rvan-mee force-pushed the fix-pointer-equal-comparison branch from c53613b to 8eb0566 Compare August 19, 2024 08:24
adds a check to see if the GEP with more then 2 operands is done on an array, if this is the case we can compare the array and its offset directly without dereferencing it
@rvan-mee rvan-mee force-pushed the fix-pointer-equal-comparison branch from 8eb0566 to 480cdb6 Compare August 19, 2024 09:59
@yuri91
Copy link
Member

yuri91 commented Aug 19, 2024

Closing this. for js we will accept undefined as an element. for wasmgc we will add a sentinel value to arrays

@yuri91 yuri91 closed this Aug 19, 2024
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.

3 participants