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

Make the memory GEP use the memory type #55412

Closed
wants to merge 2 commits into from

Conversation

Zentrik
Copy link
Member

@Zentrik Zentrik commented Aug 7, 2024

Given the comment LLVM is very bad at handling GEP with types different from the load seems like we want this. #54853 said this was changed in preparation for gep changing but it seems easy enough to make this change then.

Fixes #55090, closes #55107.

@Zentrik Zentrik changed the title Revert #54853 partially Make the memory GEP use the memory type Aug 7, 2024
@Zentrik
Copy link
Member Author

Zentrik commented Aug 7, 2024

@nanosoldier runbenchmarks("array", vs=":master")

@giordano giordano added the compiler:llvm For issues that relate to LLVM label Aug 7, 2024
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

SGTM. I hoped llvm was getting better at this

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@vtjnash
Copy link
Member

vtjnash commented Aug 8, 2024

Looks like a very mixed bag of results? Some of those are known to be unstable, but it is unclear to me why this is quite so impactful looking

@Zentrik
Copy link
Member Author

Zentrik commented Aug 8, 2024

The simd regression disappears on llvm 17 but the sparse one does not. Perhaps this change should only be made for unions? If that doesn't fix the regressions, I guess there's not much to be done.

@gbaraldi
Copy link
Member

gbaraldi commented Aug 8, 2024

It's so odd. The GEP i8 is the canonical way of doing this but what a mess.

@Zentrik
Copy link
Member Author

Zentrik commented Aug 8, 2024

Never mind, I must have got confused with some other issue, unions have nothing to do with this. Closing this then, we probably should merge #55107 even though it doesn't actually fix the regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:llvm For issues that relate to LLVM
Projects
None yet
5 participants