[NFC] Add DIExpression::calculateFragmentIntersect #97738
Merged
+132
−151
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-ir Author: Orlando Cazalet-Hyams (OCHyams) ChangesPatch [3/x] to fix structured bindings debug info in SROA. This function computes a fragment, bit-extract operation if needed, and new This generalises, simplifies, and replaces at::calculateFragmentIntersect. That The new version takes doesn't have a DbgRecord parameter, instead using an The new version has a new out-param OffsetFromLocationInBits which is set to the Based on top of #97705 and #97719. Ignore the first two commits, which are under review over there. Patch is 26.57 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/97738.diff 7 Files Affected:
diff --git a/llvm/include/llvm/IR/DbgVariableFragmentInfo.h b/llvm/include/llvm/IR/DbgVariableFragmentInfo.h
new file mode 100644
index 0000000000000..40326d5792f9f
--- /dev/null
+++ b/llvm/include/llvm/IR/DbgVariableFragmentInfo.h
@@ -0,0 +1,45 @@
+//===- llvm/IR/DbgVariableFragmentInfo.h ------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Helper struct to describe a fragment of a debug variable.
+//
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_IR_DBGVARIABLEFRAGMENTINFO_H
+#define LLVM_IR_DBGVARIABLEFRAGMENTINFO_H
+
+#include <cstdint>
+
+namespace llvm {
+struct DbgVariableFragmentInfo {
+ DbgVariableFragmentInfo() = default;
+ DbgVariableFragmentInfo(uint64_t SizeInBits, uint64_t OffsetInBits)
+ : SizeInBits(SizeInBits), OffsetInBits(OffsetInBits) {}
+ uint64_t SizeInBits;
+ uint64_t OffsetInBits;
+ /// Return the index of the first bit of the fragment.
+ uint64_t startInBits() const { return OffsetInBits; }
+ /// Return the index of the bit after the end of the fragment, e.g. for
+ /// fragment offset=16 and size=32 return their sum, 48.
+ uint64_t endInBits() const { return OffsetInBits + SizeInBits; }
+
+ /// Returns a zero-sized fragment if A and B don't intersect.
+ static DbgVariableFragmentInfo intersect(DbgVariableFragmentInfo A,
+ DbgVariableFragmentInfo B) {
+ // Don't use std::max or min to avoid including <algorithm>.
+ uint64_t StartInBits =
+ A.OffsetInBits > B.OffsetInBits ? A.OffsetInBits : B.OffsetInBits;
+ uint64_t EndInBits =
+ A.endInBits() < B.endInBits() ? A.endInBits() : B.endInBits();
+ if (EndInBits <= StartInBits)
+ return {0, 0};
+ return DbgVariableFragmentInfo(EndInBits - StartInBits, StartInBits);
+ }
+};
+} // end namespace llvm
+
+#endif // LLVM_IR_DBGVARIABLEFRAGMENTINFO_H
diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h
index 524945862e8d4..e50ff1b9f286c 100644
--- a/llvm/include/llvm/IR/DebugInfoMetadata.h
+++ b/llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -21,6 +21,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/iterator_range.h"
#include "llvm/IR/Constants.h"
+#include "llvm/IR/DbgVariableFragmentInfo.h"
#include "llvm/IR/Metadata.h"
#include "llvm/IR/PseudoProbe.h"
#include "llvm/Support/Casting.h"
@@ -2886,29 +2887,7 @@ class DIExpression : public MDNode {
/// Return whether there is exactly one operator and it is a DW_OP_deref;
bool isDeref() const;
- /// Holds the characteristics of one fragment of a larger variable.
- struct FragmentInfo {
- FragmentInfo() = default;
- FragmentInfo(uint64_t SizeInBits, uint64_t OffsetInBits)
- : SizeInBits(SizeInBits), OffsetInBits(OffsetInBits) {}
- uint64_t SizeInBits;
- uint64_t OffsetInBits;
- /// Return the index of the first bit of the fragment.
- uint64_t startInBits() const { return OffsetInBits; }
- /// Return the index of the bit after the end of the fragment, e.g. for
- /// fragment offset=16 and size=32 return their sum, 48.
- uint64_t endInBits() const { return OffsetInBits + SizeInBits; }
-
- /// Returns a zero-sized fragment if A and B don't intersect.
- static DIExpression::FragmentInfo intersect(DIExpression::FragmentInfo A,
- DIExpression::FragmentInfo B) {
- uint64_t StartInBits = std::max(A.OffsetInBits, B.OffsetInBits);
- uint64_t EndInBits = std::min(A.endInBits(), B.endInBits());
- if (EndInBits <= StartInBits)
- return {0, 0};
- return DIExpression::FragmentInfo(EndInBits - StartInBits, StartInBits);
- }
- };
+ using FragmentInfo = DbgVariableFragmentInfo;
/// Return the number of bits that have an active value, i.e. those that
/// aren't known to be zero/sign (depending on the type of Var) and which
@@ -3003,6 +2982,16 @@ class DIExpression : public MDNode {
/// return true with an offset of zero.
bool extractIfOffset(int64_t &Offset) const;
+ /// Assuming that the expression operates on an address, extract a constant
+ /// offset and the succsessive ops. Return false if the expression contains
+ /// any incompatible ops (including non-zero DW_OP_LLVM_args - only a single
+ /// address operand to the expression is permitted).
+ ///
+ /// We don't try very hard to interpret the expression because we assume that
+ /// foldConstantMath has canonicalized the expression.
+ bool extractLeadingOffset(int64_t &Offset,
+ SmallVectorImpl<uint64_t> &RemainingOps) const;
+
/// Returns true iff this DIExpression contains at least one instance of
/// `DW_OP_LLVM_arg, n` for all n in [0, N).
bool hasAllLocationOps(unsigned N) const;
@@ -3095,6 +3084,43 @@ class DIExpression : public MDNode {
return 0;
}
+ /// Computes a fragment, bit-extract operation if needed, and new constant
+ /// offset to describe a part of a variable covered by some memory.
+ ///
+ /// The memory region starts at:
+ /// \p SliceStart + \p SliceOffsetInBits
+ /// And is size:
+ /// \p SliceSizeInBits
+ ///
+ /// The location of the existing variable fragment \p VarFrag is:
+ /// \p DbgPtr + \p DbgPtrOffsetInBits + \p DbgExtractOffsetInBits.
+ ///
+ /// It is intended that these arguments are derived from a debug record:
+ /// - \p DbgPtr is the (single) DIExpression operand.
+ /// - \p DbgPtrOffsetInBits is the constant offset applied to \p DbgPtr.
+ /// - \p DbgExtractOffsetInBits is the offset from a
+ /// DW_OP_LLVM_bit_extract_[sz]ext operation.
+ ///
+ /// Results and return value:
+ /// - Return false if the result can't be calculated for any reason.
+ /// - \p Result is set to nullopt if the intersect equals \p VarFarg.
+ /// - \p Result contains a zero-sized fragment if there's no intersect.
+ /// - \p OffsetFromLocationInBits is set to the difference between the first
+ /// bit of the variable location and the first bit of the slice. The
+ /// magnitude of a negative value therefore indicates the number of bits
+ /// into the variable fragment that the memory region begins.
+ ///
+ /// We don't pass in a debug record directly to get the constituent parts
+ /// and offsets because different debug records store the information in
+ /// different places (dbg_assign has two DIExpressions - one contains the
+ /// fragment info for the entire intrinsic).
+ static bool calculateFragmentIntersect(
+ const DataLayout &DL, const Value *SliceStart, uint64_t SliceOffsetInBits,
+ uint64_t SliceSizeInBits, const Value *DbgPtr, int64_t DbgPtrOffsetInBits,
+ int64_t DbgExtractOffsetInBits, DIExpression::FragmentInfo VarFrag,
+ std::optional<DIExpression::FragmentInfo> &Result,
+ int64_t &OffsetFromLocationInBits);
+
using ExtOps = std::array<uint64_t, 6>;
/// Returns the ops for a zero- or sign-extension in a DIExpression.
diff --git a/llvm/include/llvm/IR/DebugProgramInstruction.h b/llvm/include/llvm/IR/DebugProgramInstruction.h
index ed8081a3cad19..fd93dd9b89f7c 100644
--- a/llvm/include/llvm/IR/DebugProgramInstruction.h
+++ b/llvm/include/llvm/IR/DebugProgramInstruction.h
@@ -50,6 +50,7 @@
#include "llvm/ADT/ilist.h"
#include "llvm/ADT/ilist_node.h"
#include "llvm/ADT/iterator.h"
+#include "llvm/IR/DbgVariableFragmentInfo.h"
#include "llvm/IR/DebugLoc.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/SymbolTableListTraits.h"
@@ -460,6 +461,19 @@ class DbgVariableRecord : public DbgRecord, protected DebugValueUser {
resetDebugValue(0, NewLocation);
}
+ std::optional<DbgVariableFragmentInfo> getFragment() const;
+ /// Get the FragmentInfo for the variable if it exists, otherwise return a
+ /// FragmentInfo that covers the entire variable if the variable size is
+ /// known, otherwise return a zero-sized fragment.
+ DbgVariableFragmentInfo getFragmentOrEntireVariable() const {
+ DbgVariableFragmentInfo VariableSlice(0, 0);
+ // Get the fragment or variable size, or zero.
+ if (auto Sz = getFragmentSizeInBits())
+ VariableSlice.SizeInBits = *Sz;
+ if (auto Frag = getFragment())
+ VariableSlice.OffsetInBits = Frag->OffsetInBits;
+ return VariableSlice;
+ }
/// Get the size (in bits) of the variable, or fragment of the variable that
/// is described.
std::optional<uint64_t> getFragmentSizeInBits() const;
diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index 7f1489ebbd740..d86e7c7da2d63 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -1864,181 +1864,56 @@ void at::deleteAll(Function *F) {
DVR->eraseFromParent();
}
-/// Get the FragmentInfo for the variable if it exists, otherwise return a
-/// FragmentInfo that covers the entire variable if the variable size is
-/// known, otherwise return a zero-sized fragment.
-static DIExpression::FragmentInfo
-getFragmentOrEntireVariable(const DbgVariableRecord *DVR) {
- DIExpression::FragmentInfo VariableSlice(0, 0);
- // Get the fragment or variable size, or zero.
- if (auto Sz = DVR->getFragmentSizeInBits())
- VariableSlice.SizeInBits = *Sz;
- if (auto Frag = DVR->getExpression()->getFragmentInfo())
- VariableSlice.OffsetInBits = Frag->OffsetInBits;
- return VariableSlice;
-}
-
-static DIExpression::FragmentInfo
-getFragmentOrEntireVariable(const DbgVariableIntrinsic *DVI) {
- DIExpression::FragmentInfo VariableSlice(0, 0);
- // Get the fragment or variable size, or zero.
- if (auto Sz = DVI->getFragmentSizeInBits())
- VariableSlice.SizeInBits = *Sz;
- if (auto Frag = DVI->getExpression()->getFragmentInfo())
- VariableSlice.OffsetInBits = Frag->OffsetInBits;
- return VariableSlice;
-}
+/// FIXME: Remove this wrapper function and call
+/// DIExpression::calculateFragmentIntersect directly.
template <typename T>
bool calculateFragmentIntersectImpl(
const DataLayout &DL, const Value *Dest, uint64_t SliceOffsetInBits,
uint64_t SliceSizeInBits, const T *AssignRecord,
std::optional<DIExpression::FragmentInfo> &Result) {
- // There are multiple offsets at play in this function, so let's break it
- // down. Starting with how variables may be stored in allocas:
- //
- // 1 Simplest case: variable is alloca sized and starts at offset 0.
- // 2 Variable is larger than the alloca: the alloca holds just a part of it.
- // 3 Variable is smaller than the alloca: the alloca may hold multiple
- // variables.
- //
- // Imagine we have a store to the entire alloca. In case (3) the store
- // affects bits outside of the bounds of each variable. In case (2), where
- // the alloca holds the Xth bit to the Yth bit of a variable, the
- // zero-offset store doesn't represent an assignment at offset zero to the
- // variable. It is an assignment to offset X.
- //
- // # Example 1
- // Obviously, not all stores are alloca-sized and have zero offset. Imagine
- // the lower 32 bits of this store are dead and are going to be DSEd:
- //
- // store i64 %v, ptr %dest, !DIAssignID !1
- // dbg.assign(..., !DIExpression(fragment, 128, 32), !1, %dest,
- // !DIExpression(DW_OP_plus_uconst, 4))
- //
- // Goal: Given our dead bits at offset:0 size:32 for the store, determine the
- // part of the variable, which fits in the fragment expressed by the
- // dbg.assign, that has been killed, if any.
- //
- // calculateFragmentIntersect(..., SliceOffsetInBits=0,
- // SliceSizeInBits=32, Dest=%dest, Assign=dbg.assign)
- //
- // Drawing the store (s) in memory followed by the shortened version ($),
- // then the dbg.assign (d), with the fragment information on a separate scale
- // underneath:
- //
- // Memory
- // offset
- // from
- // dest 0 63
- // | |
- // s[######] - Original stores 64 bits to Dest.
- // $----[##] - DSE says the lower 32 bits are dead, to be removed.
- // d [##] - Assign's address-modifying expression adds 4 bytes to
- // dest.
- // Variable | |
- // Fragment 128|
- // Offsets 159
- //
- // The answer is achieved in a few steps:
- // 1. Add the fragment offset to the store offset:
- // SliceOffsetInBits:0 + VarFrag.OffsetInBits:128 = 128
- //
- // 2. Subtract the address-modifying expression offset plus difference
- // between d.address and dest:
- // 128 - (expression_offset:32 + (d.address - dest):0) = 96
- //
- // 3. That offset along with the store size (32) represents the bits of the
- // variable that'd be affected by the store. Call it SliceOfVariable.
- // Intersect that with Assign's fragment info:
- // SliceOfVariable ∩ Assign_fragment = none
- //
- // In this case: none of the dead bits of the store affect Assign.
- //
- // # Example 2
- // Similar example with the same goal. This time the upper 16 bits
- // of the store are going to be DSE'd.
- //
- // store i64 %v, ptr %dest, !DIAssignID !1
- // dbg.assign(..., !DIExpression(fragment, 128, 32), !1, %dest,
- // !DIExpression(DW_OP_plus_uconst, 4))
- //
- // calculateFragmentIntersect(..., SliceOffsetInBits=48,
- // SliceSizeInBits=16, Dest=%dest, Assign=dbg.assign)
- //
- // Memory
- // offset
- // from
- // dest 0 63
- // | |
- // s[######] - Original stores 64 bits to Dest.
- // $[####]-- - DSE says the upper 16 bits are dead, to be removed.
- // d [##] - Assign's address-modifying expression adds 4 bytes to
- // dest.
- // Variable | |
- // Fragment 128|
- // Offsets 159
- //
- // Using the same steps in the first example:
- // 1. SliceOffsetInBits:48 + VarFrag.OffsetInBits:128 = 176
- // 2. 176 - (expression_offset:32 + (d.address - dest):0) = 144
- // 3. SliceOfVariable offset = 144, size = 16:
- // SliceOfVariable ∩ Assign_fragment = (offset: 144, size: 16)
- // SliceOfVariable tells us the bits of the variable described by Assign that
- // are affected by the DSE.
+ // No overlap if this DbgRecord describes a killed location.
if (AssignRecord->isKillAddress())
return false;
- DIExpression::FragmentInfo VarFrag =
- getFragmentOrEntireVariable(AssignRecord);
- if (VarFrag.SizeInBits == 0)
- return false; // Variable size is unknown.
-
- // Calculate the difference between Dest and the dbg.assign address +
- // address-modifying expression.
- int64_t PointerOffsetInBits;
- {
- auto DestOffsetInBytes =
- AssignRecord->getAddress()->getPointerOffsetFrom(Dest, DL);
- if (!DestOffsetInBytes)
- return false; // Can't calculate difference in addresses.
-
- int64_t ExprOffsetInBytes;
- if (!AssignRecord->getAddressExpression()->extractIfOffset(
- ExprOffsetInBytes))
- return false;
+ int64_t AddrOffsetInBytes;
+ SmallVector<uint64_t> PostOffsetOps; //< Unused.
+ // Bail if we can't find a constant offset (or none) in the expression.
+ if (!AssignRecord->getAddressExpression()->extractLeadingOffset(
+ AddrOffsetInBytes, PostOffsetOps))
+ return false;
+ int64_t AddrOffsetInBits = AddrOffsetInBytes * 8;
- int64_t PointerOffsetInBytes = *DestOffsetInBytes + ExprOffsetInBytes;
- PointerOffsetInBits = PointerOffsetInBytes * 8;
- }
+ Value *Addr = AssignRecord->getAddress();
+ // FIXME: It may not always be zero.
+ int64_t BitExtractOffsetInBits = 0;
+ DIExpression::FragmentInfo VarFrag =
+ AssignRecord->getFragmentOrEntireVariable();
- // Adjust the slice offset so that we go from describing the a slice
- // of memory to a slice of the variable.
- int64_t NewOffsetInBits =
- SliceOffsetInBits + VarFrag.OffsetInBits - PointerOffsetInBits;
- if (NewOffsetInBits < 0)
- return false; // Fragment offsets can only be positive.
- DIExpression::FragmentInfo SliceOfVariable(SliceSizeInBits, NewOffsetInBits);
- // Intersect the variable slice with AssignRecord's fragment to trim it down
- // to size.
- DIExpression::FragmentInfo TrimmedSliceOfVariable =
- DIExpression::FragmentInfo::intersect(SliceOfVariable, VarFrag);
- if (TrimmedSliceOfVariable == VarFrag)
- Result = std::nullopt;
- else
- Result = TrimmedSliceOfVariable;
- return true;
+ int64_t OffsetFromLocationInBits; //< Unused.
+ return DIExpression::calculateFragmentIntersect(
+ DL, Dest, SliceOffsetInBits, SliceSizeInBits, Addr, AddrOffsetInBits,
+ BitExtractOffsetInBits, VarFrag, Result, OffsetFromLocationInBits);
}
+
+/// FIXME: Remove this wrapper function and call
+/// DIExpression::calculateFragmentIntersect directly.
bool at::calculateFragmentIntersect(
const DataLayout &DL, const Value *Dest, uint64_t SliceOffsetInBits,
uint64_t SliceSizeInBits, const DbgAssignIntrinsic *DbgAssign,
std::optional<DIExpression::FragmentInfo> &Result) {
+
return calculateFragmentIntersectImpl(DL, Dest, SliceOffsetInBits,
SliceSizeInBits, DbgAssign, Result);
}
+
+/// FIXME: Remove this wrapper function and call
+/// DIExpression::calculateFragmentIntersect directly.
bool at::calculateFragmentIntersect(
const DataLayout &DL, const Value *Dest, uint64_t SliceOffsetInBits,
uint64_t SliceSizeInBits, const DbgVariableRecord *DVRAssign,
std::optional<DIExpression::FragmentInfo> &Result) {
+ // FIXME: Remove this wrapper function and call
+ // DIExpression::calculateFragmentIntersect directly.
return calculateFragmentIntersectImpl(DL, Dest, SliceOffsetInBits,
SliceSizeInBits, DVRAssign, Result);
}
diff --git a/llvm/lib/IR/DebugInfoMetadata.cpp b/llvm/lib/IR/DebugInfoMetadata.cpp
index 161a30dfb3828..5f0f620684b18 100644
--- a/llvm/lib/IR/DebugInfoMetadata.cpp
+++ b/llvm/lib/IR/DebugInfoMetadata.cpp
@@ -1760,6 +1760,45 @@ bool DIExpression::extractIfOffset(int64_t &Offset) const {
return false;
}
+bool DIExpression::extractLeadingOffset(
+ int64_t &Offset, SmallVectorImpl<uint64_t> &RemainingOps) const {
+ Offset = 0;
+ RemainingOps.clear();
+
+ auto SingleLocEltsOpt = getSingleLocationExpressionElements();
+ if (!SingleLocEltsOpt)
+ return false;
+
+ auto ExprOpEnd = expr_op_iterator(SingleLocEltsOpt->end());
+ auto ExprOpIt = expr_op_iterator(SingleLocEltsOpt->begin());
+ while (ExprOpIt != ExprOpEnd) {
+ uint64_t Op = ExprOpIt->getOp();
+ if (Op == dwarf::DW_OP_deref || Op == dwarf::DW_OP_deref_size ||
+ Op == dwarf::DW_OP_deref_type || Op == dwarf::DW_OP_LLVM_fragment ||
+ Op == dwarf::DW_OP_LLVM_extract_bits_zext ||
+ Op == dwarf::DW_OP_LLVM_extract_bits_sext) {
+ break;
+ } else if (Op == dwarf::DW_OP_plus_uconst) {
+ Offset += ExprOpIt->getArg(0);
+ } else if (Op == dwarf::DW_OP_constu) {
+ uint64_t Value = ExprOpIt->getArg(0);
+ ++ExprOpIt;
+ if (ExprOpIt->getOp() == dwarf::DW_OP_plus)
+ Offset += Value;
+ else if (ExprOpIt->getOp() == dwarf::DW_OP_minus)
+ Offset -= Value;
+ else
+ return false;
+ } else {
+ // Not a const plus/minus operation or deref.
+ return false;
+ }
+ ++ExprOpIt;
+ }
+ RemainingOps.append(ExprOpIt.getBase(), ExprOpEnd.getBase());
+ return true;
+}
+
bool DIExpression::hasAllLocationOps(unsigned N) const {
SmallDenseSet<uint64_t, 4> SeenOps;
for (auto ExprOp : expr_ops())
@@ -2052,6 +2091,75 @@ std::optional<DIExpression *> DIExpression::createFragmentExpression(
return DIExpression::get(Expr->getContext(), Ops);
}
+/// See declaration for more info.
+bool DIExpression::calculateFragmentIntersect(
+ const DataLayout &DL, const Value *SliceStart, uint64_t SliceOffsetInBits,
+ uint64_t SliceSizeInBits, const Value *DbgPtr, int64_t DbgPtrOffsetInBits,
+ int64_t DbgExtractOffsetInBits, DIExpression::FragmentInfo VarFrag,
+ std::optional<DIExpression::FragmentInfo> &Result,
+ int64_t &OffsetFromLocationInBits) {
+
+ if (VarFrag.SizeInBits == 0)
+ return false; // Variable size is unknown.
+
+ // Difference between mem slice start and the dbg location start....
[truncated]
|
OCHyams
force-pushed
the
structured-bindings-fix-3
branch
from
e1c71d8
to
6eb61bb
Compare
SLTozer
reviewed
Jul 11, 2024
llvm/lib/IR/DebugInfo.cpp
Outdated
Comment on lines
1915
to
1916
| // FIXME: Remove this wrapper function and call | ||
| // DIExpression::calculateFragmentIntersect directly. |
There was a problem hiding this comment.
Comment probably doesn't need to be repeated.
llvm/lib/IR/DebugInfo.cpp
Outdated
| return false; | ||
| int64_t AddrOffsetInBytes; | ||
| SmallVector<uint64_t> PostOffsetOps; //< Unused. | ||
| // Bail if we can't find a constant offset (or none) in the expression. |
There was a problem hiding this comment.
I think this description is a little confusing - perhaps something more like, "if we can't determine the presence or value of a constant offset in the expression"? YMMV, this is just my opinion.
llvm/lib/IR/DebugInfo.cpp
Outdated
Comment on lines
1878
to
1884
| int64_t AddrOffsetInBytes; | ||
| SmallVector<uint64_t> PostOffsetOps; //< Unused. | ||
| // Bail if we can't find a constant offset (or none) in the expression. | ||
| if (!AssignRecord->getAddressExpression()->extractLeadingOffset( | ||
| AddrOffsetInBytes, PostOffsetOps)) | ||
| return false; | ||
| int64_t AddrOffsetInBits = AddrOffsetInBytes * 8; |
There was a problem hiding this comment.
Suggested change
| int64_t AddrOffsetInBytes; | |
| SmallVector<uint64_t> PostOffsetOps; //< Unused. | |
| // Bail if we can't find a constant offset (or none) in the expression. | |
| if (!AssignRecord->getAddressExpression()->extractLeadingOffset( | |
| AddrOffsetInBytes, PostOffsetOps)) | |
| return false; | |
| int64_t AddrOffsetInBits = AddrOffsetInBytes * 8; | |
| int64_t AddrOffsetInBits; | |
| { | |
| int64_t AddrOffsetInBytes; | |
| SmallVector<uint64_t> PostOffsetOps; //< Unused. | |
| // Bail if we can't find a constant offset (or none) in the expression. | |
| if (!AssignRecord->getAddressExpression()->extractLeadingOffset( | |
| AddrOffsetInBytes, PostOffsetOps)) | |
| return false; | |
| AddrOffsetInBits = AddrOffsetInBytes * 8; | |
| } |
Minor style suggestion, feel free to ignore.
llvm/lib/IR/DebugInfoMetadata.cpp
Outdated
| int64_t MemEndRelToFragInBits = MemStartRelToFragInBits + SliceSizeInBits; | ||
| // If the memory region starts before the debug location the fragment | ||
| // offset would be negative, which we can't encode. Limit those to 0. This | ||
| // is fine becausethose bits necessarily don't overlap with the variable |
There was a problem hiding this comment.
Suggested change
| // is fine becausethose bits necessarily don't overlap with the variable | |
| // is fine because those bits necessarily don't overlap with the variable |
| // | | ||
| // mem slice start: SliceOfVariable offset=40 | ||
| int64_t MemStartRelToFragInBits = | ||
| MemStartRelToDbgStartInBits + VarFrag.OffsetInBits; |
There was a problem hiding this comment.
I feel like the term RelToFrag is misleading, I feel that implies relative to the current debug fragment rather than to the variable as a whole?
There was a problem hiding this comment.
Nice catch. Change Frag to Var. Still not an amazing name, but I found coming up with meaningful names in this function was pretty difficult (hence all the comments!). I'm happy to take further suggestions?
There was a problem hiding this comment.
No better suggestions unfortunately, "Var" was the best I had too. I think it's reasonable, it's just slightly nonsensical in that the variable isn't a contiguous region of memory that the memory slice can exist relative to, but it at least makes sense in terms of how we ultimately calculate the variable fragment covered by the slice.
Patch [3/x] to fix structured bindings debug info in SROA. This function computes a fragment, bit-extract operation if needed, and new constant offset to describe a part of a variable covered by some memory. This generalises, simplifies, and replaces at::calculateFragmentIntersect. That version is still used as a wrapper for now though to keep this change NFC. The new version takes doesn't have a DbgRecord parameter, instead using an explicit address and address offset. The old version only operates on dbg_assigns and this change means it can also operate on dbg_declare records easily, which it will do in a subsequent patch. The new version has a new out-param OffsetFromLocationInBits which is set to the difference between the first bit of the variable location and the first bit of the memory slice. This will be used in a subsequent patch in SROA to determine the new offset to use in the address expression after splitting an alloca.
OCHyams
force-pushed
the
structured-bindings-fix-3
branch
from
8ab7713
to
f8875e3
Compare
OCHyams
commented
Jul 11, 2024
| // | | ||
| // mem slice start: SliceOfVariable offset=40 | ||
| int64_t MemStartRelToFragInBits = | ||
| MemStartRelToDbgStartInBits + VarFrag.OffsetInBits; |
There was a problem hiding this comment.
Nice catch. Change Frag to Var. Still not an amazing name, but I found coming up with meaningful names in this function was pretty difficult (hence all the comments!). I'm happy to take further suggestions?
SLTozer
approved these changes
Jul 11, 2024
| // | | ||
| // mem slice start: SliceOfVariable offset=40 | ||
| int64_t MemStartRelToFragInBits = | ||
| MemStartRelToDbgStartInBits + VarFrag.OffsetInBits; |
There was a problem hiding this comment.
No better suggestions unfortunately, "Var" was the best I had too. I think it's reasonable, it's just slightly nonsensical in that the variable isn't a contiguous region of memory that the memory slice can exist relative to, but it at least makes sense in terms of how we ultimately calculate the variable fragment covered by the slice.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/50/builds/1010 Here is the relevant piece of the build log for the reference: |
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
Patch [3/x] to fix structured bindings debug info in SROA. This function computes a fragment, bit-extract operation if needed, and new constant offset to describe a part of a variable covered by some memory. This generalises, simplifies, and replaces at::calculateFragmentIntersect. That version is still used as a wrapper for now though to keep this change NFC. The new version takes doesn't have a DbgRecord parameter, instead using an explicit address and address offset. The old version only operates on dbg_assigns and this change means it can also operate on dbg_declare records easily, which it will do in a subsequent patch. The new version has a new out-param OffsetFromLocationInBits which is set to the difference between the first bit of the variable location and the first bit of the memory slice. This will be used in a subsequent patch in SROA to determine the new offset to use in the address expression after splitting an alloca.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Patch [3/x] to fix structured bindings debug info in SROA.
This function computes a fragment, bit-extract operation if needed, and new
constant offset to describe a part of a variable covered by some memory.
This generalises, simplifies, and replaces at::calculateFragmentIntersect. That
version is still used as a wrapper for now though to keep this change NFC.
The new version takes doesn't have a DbgRecord parameter, instead using an
explicit address and address offset. The old version only operates on
dbg_assigns and this change means it can also operate on dbg_declare records
easily, which it will do in a subsequent patch.
The new version has a new out-param OffsetFromLocationInBits which is set to the
difference between the first bit of the variable location and the first bit of
the memory slice. This will be used in a subsequent patch in SROA to determine
the new offset to use in the address expression after splitting an alloca.
Based on top of #97705 and #97719.
Ignore the first two commits, which are under review over there.(rebased)