[PATCH] D71828: [InstCombine] Convert vector store to scalar store if only one element updated

[Qiu Chaofan via Phabricator via llvm-commits]

qiucf added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1204
+         BBI != SI.getIterator(); ++BBI)
+      if (AA->getModRefInfo(&*BBI, MemoryLocation::get(&SI)) == ModRefInfo::Mod)
+        return nullptr;
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > 1. Is it bitfield? Should we be checking `if (AA->getModRefInfo(&*BBI, MemoryLocation::get(&SI) & ModRefInfo::Mod)` ?
> > 2. This does correctly work for calls right, not just instructions?
> This looks better, but i don't have prior expirience with `ModRefInfo`,
> so this still looks suspicious to me - is this conservatively correct?
> `isModSet()` looks for `MustMod`, which seems stronger than `Mod`?
hmm.. As the doc/comment says, I think this bit indicates that the mem location may/must be modified, and there's another bit about it's a '//must//' or a '//may//'. So the three bit should each mean `May Mod Ref`. 

So `Mod` means it may write and `mustAlias` **not** found, while `MustMod` means it may write and `mustAlias` found. Here we only need to care if it may write or not (the bit), regardless of alias.

Maybe I'm not correct. I found a related commit, rG50db8a, for a look :)


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1201-1203
+    // Make sure memory isn't modified between the two.
+    for (BasicBlock::iterator BBI = Load->getIterator();
+         BBI != SI.getIterator(); ++BBI)
----------------
lebedev.ri wrote:
> Hm, how do we know both the load and the store are in the same basic block?
We checked parent of `Load` and `SI` above, if they aren't the same, exit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71828