-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
remove memset calls when setting to undefined in unsafe modes #14104
Conversation
Using the ❯ ./stage3/bin/zig build-obj -OReleaseFast test.zig && objdump -d test.o
test.o: file format mach-o arm64
Disassembly of section __TEXT,__text:
0000000000000000 <ltmp0>:
0: c0 03 5f d6 ret |
f606b1d
to
45c3181
Compare
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.
I think this should be further down in the pipeline, because setting things to undefined is relevant for optimization passes. With the memset to undefined removed, this may be preventing later passes from knowledge that they otherwise had access to.
That's a good point, I'll have to look more into why LLVM isn't making this optimization (I can't remember if I was even using the LLVM backend during testing). But, what you're saying is that this should be moved into the backend specific codegens? |
Yes, exactly. |
How should this optimization handle volatile pointers? It's my understanding that reads/writes to volatile pointers cannot be optimized out even in optimized modes. |
That's a great question. Volatile means that a load or a store through that pointer has side effects. Storing undefined through a volatile pointer is almost certainly a bug. I think the language spec should make it illegal to do that. This means that the frontend should check for this scenario and emit a compile error (and additionally add runtime safety for it if possible), and the backends can assert or assume that this will never happen. |
1a067e8
to
2310b66
Compare
Superseded by #15278. |
I'm not too familiar with the codegen pipeline yet, so I'm not sure if this should (can?) be moved further up into AstGen.
closes #14094