[PATCH] D66167: MemTag: stack initializer merging.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc accepted this revision.
pcc added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:196
+      if (Offset > LastOffset)
+        emitZeroes(IRB, LastOffset, Offset - LastOffset);
+
----------------
eugenis wrote:
> pcc wrote:
> > This could call `emitUndef`, right? Is there an advantage in using `STZG` instead of `STG` here, and if so, should we be doing the same thing on line 181?
> No, because memset(0) does not update Out[] (see applyMemset), so we don't know if the tail is zero or undef. Line 181 is different - with Ranges.empty() we know that the entire allocation is undef.
> 
> If we wanted to change this, Out[] would need to remember which bytes have been written to in a separate mask.
> 
> I did not do this because STG and STGZ are expected to have approximately same overhead.
I see. I'd probably leave a comment about that here.

I guess an alternative would be to eliminate this special case by also making memset(0) update Out. Probably doesn't matter much for now, though.


================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:253
+                                      VecTy->getNumElements());
+        V = IRB.CreatePointerCast(V, NewTy);
+      }
----------------
Is there test coverage for this code? Please add if not.


================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:338
+
+    if (isNoModRef(AA->getModRefInfo(&*BI, AllocaLoc))) {
+      continue;
----------------
Remove braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66167