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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D71828#1794697 <https://reviews.llvm.org/D71828#1794697>, @RKSimon wrote:

> @spatel @jdoerfert Should/could we use attributor to keep track of the 'storeable' memory range?


For simple examples like this we already do. Run the Attributor on them and you get `dereferenceable` annotations for the argument pointers.
Nevertheless, we can lose information. My suggestions to keep it is to emit something like this:

  `call void @llvm.assume(i1 true) ["derefereceable"(%ptr, sizeof(access-we-just-removed))]`

I do have a RFC on the mailing list on this topic but no conclusion yet. If we decide to go forward with it we start emitting them and I
we write a combiner pass as well as integration into things like the Attributor to use the information.

Long story short, I think doing this is fine, once we have better assume capabilities we should emit one to keep the information about dereferenceability.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1196
+    if (!Load->isSimple() ||
+        Load->getPointerOperand() != SI.getPointerOperand())
+      return nullptr;
----------------
lkail wrote:
> Not an expert of `opt`, I just wonder is it more proper to check if `Load->getPointerOperand()` `MustAlias` `SI.getPointerOperand()`, since there might be `bitcast`s of these pointers?
I'd just call `stripPointerCastSameRepresentation()` on both pointers. That should give you all cases you care about (wrt. casts). Though, the `match` above might take care of it actually.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1216
+    return new StoreInst(NewElement, GEP);
+  }
+
----------------
The GEP should be `inbounds` and the store should keep the `nontemporal` flag if present on the original.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71828