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

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added a comment.

Overall I think this looks fine, so thanks for addressing the issue. I had one small comment and one question I left inline.

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

You shouldn't consider any of these blocking.



================
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) {
----------------
Why does this take an `optional<>` as an output argument and return `bool`? wouldn't it make more sense to just return an `optional<>`? 


================
Comment at: llvm/lib/IR/DebugInfo.cpp:1902
+  {
+    auto DestOffsetInBytes = llvm::isPointerOffset(Dest, DAI->getAddress(), DL);
+    if (!DestOffsetInBytes)
----------------
I know `isPointerOffset()` isn't part of this patch, but it's very surprising to find out that it doesn't return `bool`.


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

https://reviews.llvm.org/D148536