[PATCH] D147696: [Assignment Tracking][SROA] Fix fragment when slice size equals variable size

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:153-160
+  // If there's no fragment, we need to ensure that any new fragment doesn't
+  // cover the whole variable. We hit this codepath when an alloca which
+  // contains contiguous distinct variables is split. e.g.
+  // alloca i64 with two linked 32-bit variables, one at offset 0 and the
+  // other at offset 32. When split into two 32 bit allocas we don't want
+  // to introduce a (DW_OP_fragment 0, 32) or (DW_OP_fragment 32, 32) as
+  // those fragments are the same size as the variables, which is a verifier
----------------
jmorse wrote:
> I feel this could be more terse, something like "If this slice extracts the entirety of an independent variable from a larger alloca, do not produce a fragment expression, as the variable is not fragmented". IMO it's better to be terse than to give too much detail, YMMV.
SGTM


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:262-267
+      switch (Result) {
+      case UseNoFrag:
+        break;
+      case Skip:
         continue;
+      case UseFrag:
----------------
jmorse wrote:
> IMO: using if UseNoFrag ...else if Skip... would allow UseNoFrag and Skip to be handled up front, leaving UseFrag as the only onwards path which avoids indenting the rest of the block below. We can rely on optimisations to make this just as fast as a switch, IMO.
Both `UseNoFrag` and `UseFrag` need to reach code outside the outer if (`f (IsSplit`)), but we only want the fragment-making code to occur when we `UseFrag`. I've restructured it to avoid a new additional layer of indentation - are you happy with this?


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/sroa/var-sized-fragment.ll:21
+
+; CHECK: dbg.value(metadata i32 %.sroa.0.0.extract.trunc, metadata ![[#]], metadata !DIExpression())
+
----------------
jmorse wrote:
> Does the upper 32 bits not get a dbg.value(poison) at least? Best to test for that not having an expression if it does get generated, as that's the purpose of this test.
Unfortunately the upper 32 bits (which are tracked with a dbg.declare) get incorrect debug-info. See https://github.com/llvm/llvm-project/issues/61981. This issue is independent of assignment tracking.


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

https://reviews.llvm.org/D147696