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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:262-267
+      switch (Result) {
+      case UseNoFrag:
+        break;
+      case Skip:
         continue;
+      case UseFrag:
----------------
Orlando wrote:
> 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?
SGTM, it was only the extra indentation that bothered me.


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

Sure, but the fragment information for that is orthogonal surely, we can test for the wrong location but right fragment information, with a "FIXME" saying the location is wrong and a link to the bug? That'll be updated when the bug is fixed, and means we still achieve coverage of the upper bits of this alloca that's being split up either way.


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

https://reviews.llvm.org/D147696