[PATCH] D60659: [InstCombine] Eliminate stores to constant memory

Mon Apr 15 11:37:55 PDT 2019

nicholas added a comment.

In D60659#1466869 <https://reviews.llvm.org/D60659#1466869>, @reames wrote:

> In D60659#1465605 <https://reviews.llvm.org/D60659#1465605>, @nicholas wrote:
>
> > 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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60659/new/

https://reviews.llvm.org/D60659