This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine][OpaquePtr] Do not try to match store type to value type for opaque stores
AbandonedPublic

Authored by jrbyrnes on Mar 30 2023, 12:05 PM.

Details

Reviewers
nikic
arsenm
Summary

Based on https://github.com/llvm/llvm-project/commit/816d26fe5eeae4adcb0ceda05884747e15f55bb2, it seems the original intent of combineStoreToValueType is to avoid unnecessary bitcasts required to match ptr types of stores. Opaque pointers solves this issue, thus we should not perform this optimization on opaque stores. This optimization currently causes an issue since MergedLoadStoreMotion and SimplifyCFG optimizations are based on finding corresponding ops in different branches -- if the type of stores are conflicting, the optimizations will miss. We should probably not require type matching on memory ops in these passes, but we should first remove typed ptr handling where it is not needed.

Diff Detail

Event Timeline

jrbyrnes created this revision.Mar 30 2023, 12:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 12:05 PM
jrbyrnes requested review of this revision.Mar 30 2023, 12:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 12:05 PM
nikic requested changes to this revision.Mar 30 2023, 12:35 PM

I don't think your characterization here is correct. The point of the transform is to remove the bitcast of the stored value. With typed pointers, this shifted the bitcast from the stored value to the pointer. With opaque pointers, it removes the bitcast entirely. As such, I don't really understand the argument for why we should stop making this transform with opaque pointers. If you look through the test diffs, this patch is causing many optimization regressions.

Besides, even for your motivating case, this is only going to shift the problem in the different direction: What if you had a bitcasted i16 store and a half store to start with, and this transform made sure the types are the same? It's trading one case for another.

We should probably not require type matching on memory ops in these passes

This would be the correct way to address this.

This revision now requires changes to proceed.Mar 30 2023, 12:35 PM
jrbyrnes abandoned this revision.Mar 30 2023, 12:53 PM

@nikic -- thanks for your thoughts on this. Since you disagree with my characterization of the function, and that was the basis of this patch, I'll abandon in favor of a patch addressing from the opposite direction (e.g. MLSM).