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

[Jeremy Morse via Phabricator via llvm-commits]

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

I've got some quibbles about the code layout and wording, but otherwise this LGTM.



================
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
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:262-267
+      switch (Result) {
+      case UseNoFrag:
+        break;
+      case Skip:
         continue;
+      case UseFrag:
----------------
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.


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/sroa/var-sized-fragment.ll:7
+
+;; NOTE: The upper 32 bits are currenlty
+;; not tracked with assignment tracking (due to the dbg.declare containing an
----------------



================
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())
+
----------------
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.


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

https://reviews.llvm.org/D147696