Skip to content

[RemoveDIs] Update ConvertDebugDeclareToDebugValue after #72276 #73508

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

OCHyams
Copy link
Contributor

@OCHyams OCHyams commented Nov 27, 2023

That patch was missing a recent change to the non-DPValue StoreInst overload. Add it to the DPValue version.

This is tested by llvm/tests/Transforms/Mem2Reg/dbg_declare_to_value_conversions.ll, which already has --try-experimental-debuginfo-iterators. It doesn't fail currently because until #74090 lands the declares are not converted to DPValues. See that review for a note on testing and patch dependencies.

That patch was missing a recent change to the non-DPValue StoreInst
overload. Add it to the DPValue version.

This codepath will be tested after SROA is updated to handle DPValues.
@llvmbot
Copy link
Member

llvmbot commented Nov 27, 2023

@llvm/pr-subscribers-llvm-transforms

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

That patch was missing a recent change to the non-DPValue StoreInst overload. Add it to the DPValue version.

This codepath will be tested after SROA is updated to handle DPValues.


Full diff: https://github.com/llvm/llvm-project/pull/73508.diff

1 Files Affected:

  • (modified) llvm/lib/Transforms/Utils/Local.cpp (+27-13)
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index e399329a58873e7..9053f891c40aa04 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -1772,23 +1772,37 @@ void llvm::ConvertDebugDeclareToDebugValue(DPValue *DPV, StoreInst *SI,
 
   DebugLoc NewLoc = getDebugValueLocDPV(DPV);
 
-  if (!valueCoversEntireFragment(DV->getType(), DPV)) {
-    // FIXME: If storing to a part of the variable described by the dbg.declare,
-    // then we want to insert a DPValue.value for the corresponding fragment.
-    LLVM_DEBUG(dbgs() << "Failed to convert dbg.declare to DPValue: " << *DPV
-                      << '\n');
-    // For now, when there is a store to parts of the variable (but we do not
-    // know which part) we insert an DPValue record to indicate that we know
-    // nothing about the variable's content.
-    DV = UndefValue::get(DV->getType());
-    ValueAsMetadata *DVAM = ValueAsMetadata::get(DV);
-    DPValue *NewDPV = new DPValue(DVAM, DIVar, DIExpr, NewLoc.get());
-    SI->getParent()->insertDPValueBefore(NewDPV, SI->getIterator());
+  // If the alloca describes the variable itself, i.e. the expression in the
+  // dbg.declare doesn't start with a dereference, we can perform the
+  // conversion if the value covers the entire fragment of DII.
+  // If the alloca describes the *address* of DIVar, i.e. DIExpr is
+  // *just* a DW_OP_deref, we use DV as is for the dbg.value.
+  // We conservatively ignore other dereferences, because the following two are
+  // not equivalent:
+  //     dbg.declare(alloca, ..., !Expr(deref, plus_uconstant, 2))
+  //     dbg.value(DV, ..., !Expr(deref, plus_uconstant, 2))
+  // The former is adding 2 to the address of the variable, whereas the latter
+  // is adding 2 to the value of the variable. As such, we insist on just a
+  // deref expression.
+  bool CanConvert =
+      DIExpr->isDeref() || (!DIExpr->startsWithDeref() &&
+                            valueCoversEntireFragment(DV->getType(), DPV));
+  if (CanConvert) {
+    insertDbgValueOrDPValue(Builder, DV, DIVar, DIExpr, NewLoc,
+                            SI->getIterator());
     return;
   }
 
+  // FIXME: If storing to a part of the variable described by the dbg.declare,
+  // then we want to insert a dbg.value for the corresponding fragment.
+  LLVM_DEBUG(dbgs() << "Failed to convert dbg.declare to dbg.value: " << *DPV
+                    << '\n');
   assert(UseNewDbgInfoFormat);
-  // Create a DPValue directly and insert.
+
+  // For now, when there is a store to parts of the variable (but we do not
+  // know which part) we insert an dbg.value intrinsic to indicate that we
+  // know nothing about the variable's content.
+  DV = UndefValue::get(DV->getType());
   ValueAsMetadata *DVAM = ValueAsMetadata::get(DV);
   DPValue *NewDPV = new DPValue(DVAM, DIVar, DIExpr, NewLoc.get());
   SI->getParent()->insertDPValueBefore(NewDPV, SI->getIterator());

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooofff, my bad, good catch. Unfortunately this is a limitation of our testing approach -- there are a number of things around the edges which don't actually appear in a stage2clang build or similar.

(LGTM)

@OCHyams OCHyams merged commit c4e764e into llvm:main Dec 12, 2023
@OCHyams OCHyams deleted the ddd/local2 branch January 5, 2024 10:25
ichaer added a commit to ichaer/llvm-project-onesided_lower_bound that referenced this pull request Jan 29, 2024
* main: (5908 commits)
  [readtapi] Cleanup printing command line options (llvm#75106)
  [flang] Fix compilation error due to variable no being used (llvm#75210)
  [C API] Add getters and setters for fast-math flags on relevant instructions (llvm#75123)
  [libc++][CI] Tests the no RTTI configuration. (llvm#65518)
  [RemoveDIs] Fold variable into assert, it's only used once. NFC
  [RemoveDI] Handle DPValues in SROA (llvm#74089)
  [AArch64][GlobalISel] Test Pre-Commit for Look into array's element
  [mlir][tensor] Fix bug in `tensor.extract(tensor.from_elements)` folder (llvm#75109)
  [analyzer] Move alpha checker EnumCastOutOfRange to optin (llvm#67157)
  [RemoveDIs] Handle DPValues in replaceDbgDeclare (llvm#73507)
  [SHT_LLVM_BB_ADDR_MAP] Implements PGOAnalysisMap in Object and ObjectYAML with tests.
  [X86][GlobalISel] Add instruction selection for G_SELECT (llvm#70753)
  [AMDGPU] Remove unused function splitScalar64BitAddSub
  [LLVM][DWARF] Add compilation directory and dwo name to TU in dwo section (llvm#74909)
  [clang][Interp] Implement __builtin_ffs (llvm#72988)
  [RemoveDIs] Update ConvertDebugDeclareToDebugValue after llvm#72276 (llvm#73508)
  [libc][NFC] Reuse `FloatProperties` constant instead of creating new ones (llvm#75187)
  [RemoveDIs] Fix removeRedundantDdbgInstrs utils for dbg.declares (llvm#74102)
  Reapply "[RemoveDIs][NFC] Find DPValues using findDbgDeclares  (llvm#73500)"
  [GitHub] Remove author association print from new-prs workflow
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants