[PATCH] D118549: apply two more cases for store combine

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

I agree that pre-committed test cases would help here.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7561
+  uint64_t OffsetMin = UINT64_MAX;
+  for (auto *Store : Stores) {
+    // All the stores store different parts of the CombinedValue. A truncate is
----------------
It'd be nice if we could be cleverer about this somehow as this is 90% copy/paste from below. Seems like we only use `Offset` as a key into a map in the following loop. Can we post-process that map instead?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7732
+  if (Use2St16) {
+    SDValue SourceValueL;
+    SDValue SourceValueH;
----------------
Don't need to pre-declare these variables uninitialized like this.


================
Comment at: llvm/test/CodeGen/RISCV/store-combine.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV32
----------------
We normally check RV32 and RV64 in the same test. I think this change benefits RV64 too, right?


================
Comment at: llvm/test/CodeGen/RISCV/store-combine.ll:79
+
+!4 = !{!5, !6, i64 0}
+!5 = !{!"ab", !6, i64 0, !6, i64 1, !6, i64 2, !6, i64 3}
----------------
Do we need all of this metadata for the test to work? I presume we don't need TBAA since the stores trivially don't alias.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118549