[PATCH] D148536: [Assignment Tracking] Fix fragment error for some DSE-shortened stores

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

Orlando marked 3 inline comments as done.
Orlando added a comment.

Thank you both for the reviews!

> Also, the issue was found building Fuchsia. We share infrastructure w/ Chromium, so the URL may have been misleading.

Aha, I see, I will fix that in the commit message, thanks.



================
Comment at: llvm/include/llvm/IR/DebugInfo.h:227-235
+/// Calculate the fragment of the variable in \p DAI covered
+/// from (Dest + SliceOffsetInBits) to
+///   to (Dest + SliceOffsetInBits + SliceSizeInBits)
+///
+/// Return false if it can't be calculated for any reason.
+/// Result is set to nullopt if the intersect equals the variable fragment (or
+/// variable size) in DAI.
----------------
jmorse wrote:
> Is there a meaningful distinction between a zero-sized fragment and returning false? (just curious)
Yeah - if the intersect can't be calculated for some reason you may wish to do something different (defensive/safe). Here's the code from DeadStoreElimination:

```
    if (!at::calculateFragmentIntersect(DL, OriginalDest, DeadSliceOffsetInBits,
                                        DeadSliceSizeInBits, DAI,
                                        NewFragment) ||
        !NewFragment) {
      // We couldn't calculate the intersecting fragment for some reason. Be
      // cautious and unlink the whole assignment from the store.
      DAI->setKillAddress();
      DAI->setAssignId(GetDeadLink());
      continue;
    }
    // No intersect.
    if (NewFragment->SizeInBits == 0)
      continue;
```

In this case we unlink the existing dbg.assign if the intersect couldn't be calculated (return false) to be "safe", or if the entire intersected fragment == the existing fragment (NewFragment == nullopt). In the case of no intersect (NewFragment->SizeInBits == 0) we don't do anything to the the dbg.assign, nor insert any new ones.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:1797-1800
+bool at::calculateFragmentIntersect(
+    const DataLayout &DL, const Value *Dest, uint64_t SliceOffsetInBits,
+    uint64_t SliceSizeInBits, const DbgAssignIntrinsic *DAI,
+    std::optional<DIExpression::FragmentInfo> &Result) {
----------------
paulkirth wrote:
> Why does this take an `optional<>` as an output argument and return `bool`? wouldn't it make more sense to just return an `optional<>`? 
It does feel a bit clunky but I wanted to distinguish between "the entire fragment or variable from DAI" (`Result == nullopt`) and "the intersect couldn't be calculated for some reason" (`return false`).

Using `nullopt` to represent the entire fragment or variable from DAI isn't completely weird because debug intrinsics containing no fragment info in their `DIExpression`s are considered to represent a location for the entire variable, and `DIExpression::getFragment` returns an optional<>.

Returning optional<> would result in a less clunky interface here. We'd have to move the "is the fragment the same size as DAI's fragment or variable" check done by the caller after instead (which is not unreasonable).

As you said you didn't intend your comments to be blocking please let me know if you'd like to see this change and I'll happily create another patch.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:1902
+  {
+    auto DestOffsetInBytes = llvm::isPointerOffset(Dest, DAI->getAddress(), DL);
+    if (!DestOffsetInBytes)
----------------
paulkirth wrote:
> I know `isPointerOffset()` isn't part of this patch, but it's very surprising to find out that it doesn't return `bool`.
It does feel like the name is slightly misleading. Maybe `getPointerOffset` would make more sense.


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/dse/shorten.ll:45
   call void @llvm.dbg.assign(metadata i1 undef, metadata !11, metadata !DIExpression(), metadata !16, metadata ptr %local, metadata !DIExpression()), !dbg !17
-  call void @llvm.lifetime.start.p0i8(i64 80, ptr nonnull %local) #5, !dbg !18
   %arraydecay = getelementptr inbounds [20 x i32], ptr %local, i64 0, i64 0, !dbg !19
----------------
jmorse wrote:
> Reason for deleting these?
Ah I forgot to submit this inline comment after updating the patch. My reasoning was that I was in the neighbourhood, reading the test, dealing with failures while working on my patch. The lifetime intrinsics don't add anything and just clutter the tests. I've left the change out of this patch - I can always make the change separately.


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

https://reviews.llvm.org/D148536