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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

Hm, i don't fully recall the story here, but @qiucf, are you still interested in this?
I think this looks about right now, with some nits.

@spatel, @jdoerfert: any other thoughts?



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1206
+             m_InsertElement(m_Instruction(Source), m_Value(NewElement),
+                             m_Constant(Idx))))
+    return nullptr;
----------------
We indeed can't easily (if at all?) handle non-constant case.
But there are no tests that we don't transform when the idx is a variable.

Likewise i suspect constantexprs will give is trouble here, so i strongly recommend using `m_ConstantInt()`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1209
+
+  if (auto *Load = dyn_cast<LoadInst>(Source)) {
+    Value *SrcAddr = Load->getPointerOperand()->stripPointerCasts();
----------------
I think we are better-off early-returning instead.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1224
+        IC.Builder.CreateInBoundsGEP(NewElement->getType(), ElePtr, Idx);
+    StoreInst *NSI = new StoreInst(NewElement, GEP);
+    NSI->copyMetadata(SI, {LLVMContext::MD_nontemporal});
----------------
Needs explicit `Align(1)` now.
Though i think at least for constant offsets we could deduce the alignment,
something close to:
```
        commonAlignment(
            SI.getAlign(),
            Idx->getUniqueInteger().getLimitedValue() *
                (NewElement->getType()->getScalarSizeInBits() / 8))
```
But that can be done in a followup patch,


================
Comment at: llvm/test/Transforms/InstCombine/load-insert-store.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -S -instcombine < %s | FileCheck %s
 
----------------
I wasn't sure whether or not this is endian-sensitive, alive2 says it's not, but maybe we should be proactive and do
```
; RUN: opt < %s -instcombine -S -data-layout=e | FileCheck %s --check-prefix=ALL --check-prefixes=CHECK,LE
; RUN: opt < %s -instcombine -S -data-layout=E | FileCheck %s --check-prefix=ALL --check-prefixes=CHECK,BE
```



================
Comment at: llvm/test/Transforms/InstCombine/load-insert-store.ll:4
 
 define void @insert_store(<16 x i8>* %q, i8 zeroext %s) {
 ; CHECK-LABEL: @insert_store(
----------------
Add same test but with `<8 x i16>` (so it's not byte-sized)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71828