This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Eliminate stores to constant memory
ClosedPublic

Authored by reames on Apr 13 2019, 2:54 PM.

Details

Summary

If we have a store to a piece of memory which is known constant, then we know the store must be storing back the same value. As a result, the store (or memset, or memmove) must either be down a dead path, or a noop. In either case, it is valid to simply remove the store.

The motivating case for this involves a memmove to a buffer which is constant down a path which is dynamically dead.

Note that I'm choosing to implement the less aggressive of two possible semantics here. We could simply say that the store *is undefined*, and prune the path. That seemed a bit too likely to break real code in subtle ways, so I decide to just remove the store itself.

Diff Detail

Event Timeline

reames created this revision.Apr 13 2019, 2:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2019, 2:54 PM
reames updated this revision to Diff 195043.Apr 13 2019, 3:13 PM
reames edited the summary of this revision. (Show Details)

Rebase on top of test changes

The two statements:

We could simply say that the store *is undefined*, and prune the path.

and

The motivating case for this involves a memmove to a buffer which is constant down a path which is dynamically dead.

conspire to make me think that pruning the whole path really is the right thing to do. This might be the only hint the compiler has that the path is dead code.

Why do you think a store through constant would break subtly? For most users I'd expect a store to a constant to correspond with a write to a read-only page causing a loud crash.

Why do you think a store through constant would break subtly? For most users I'd expect a store to a constant to correspond with a write to a read-only page causing a loud crash.

I assume you mean before this patch right? If so, then yes and no. It's possible the constant location is in a readonly page, but I have a lot of downstream constant locations which are intermixed with writeable data. I'm sure other frontends might have the same. Your reasoning may hold specifically for global constants though.

Why do you think a store through constant would break subtly? For most users I'd expect a store to a constant to correspond with a write to a read-only page causing a loud crash.

I assume you mean before this patch right? If so, then yes and no. It's possible the constant location is in a readonly page, but I have a lot of downstream constant locations which are intermixed with writeable data. I'm sure other frontends might have the same. Your reasoning may hold specifically for global constants though.

I was just surprised at the claim that this might be a common unnoticed bug. Putting constants in read only memory is a technique decades older than UBSan and I regard anything the latest UBSan can catch is something that we're safe to optimize on.

I don't think this patch is wrong per se, I'm just quite confident it's safe to make it stronger. If you don't agree, you can leave the stronger optimization for another author in the future. Either way, separate patches would make it easier to find the cause of a regression.

apilipenko accepted this revision.Apr 22 2019, 10:48 AM

LGTM. I agree that we can leave the stronger variant for the future.

This revision is now accepted and ready to land.Apr 22 2019, 10:48 AM
This revision was automatically updated to reflect the committed changes.

hi reames, I have a question in the view of customer. If he writes deliberately to a read-only memory to expect trigger a seg fault, or if he wants to write to a memory which he does not realize it is read only. Should compiler tell him the writing is invalid, rather than treat it as a nop silently?

hi reames, I have a question in the view of customer. If he writes deliberately to a read-only memory to expect trigger a seg fault, or if he wants to write to a memory which he does not realize it is read only. Should compiler tell him the writing is invalid, rather than treat it as a nop silently?

See the discussion in the commit thread about this being undefined behaviour and possibilities for sanitization.