[PATCH] D28875: AArch64LoadStoreOptimizer: Update kill flags when merging stores

[Matthias Braun via Phabricator via llvm-commits]

MatzeB marked 4 inline comments as done.
MatzeB added a comment.

Thanks for the review.

The thing with Kill flags is that we want to get rid of them long term. Today they are optional: The last use of a value may or may not have a kill flag set, but if there is a kill flag, then the value must be killed at that point.
This patch fixes situations in which invalid kill flags are produced, it can remove more kill flags than necessary or not adding possible new ones; But that is fine and keeps the code simpler.



================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:695
+    if (!MergeForward) {
+      // Clear kill flags on store if moving upwards. Example:
+      //   STRWui %w0, ...
----------------
junbuml wrote:
> As far as I see, only store pair seem to be the issue, but just want to confirm if there is any case for load pair  ? 
Paired loads are fine, the loaded values are defs and the base register is taken from the place where the final paired instruction will end up (`const MachineOperand &BaseRegOp = MergeForward ? getLdStBaseOp(*Paired) : getLdStBaseOp(*I);`) so the kill flags are correct for that.


================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:697
+      //   STRWui %w0, ...
+      //   USE %w1
+      //   STRWui kill %w1  ; need to clear kill flag when moving STRWui upwards
----------------
junbuml wrote:
> Don't we need to set kill on it ? 
Kills are optional (see above)


================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:699-700
+      //   STRWui kill %w1  ; need to clear kill flag when moving STRWui upwards
+      RegOp0.setIsKill(false);
+      RegOp1.setIsKill(false);
+    } else {
----------------
junbuml wrote:
> If there is no instruction using RegOp0 and RegOp1 between stores, no need to clear kill.
Yes there is no need, but it doesn't hurt either and keeps the code slightly simpler.


================
Comment at: test/CodeGen/AArch64/ldst-opt.mir:132
+# CHECK: %w2 = COPY %w1
+# CHECK: STPWi %w1, killed %w2, killed %x0, 0
----------------
junbuml wrote:
> Don't we need to set kill on w1 here ? 
Kills are optional (see above).


Repository:
  rL LLVM

https://reviews.llvm.org/D28875