[SROA] Fix debug locations for variables with non-zero offsets #97750
Conversation
This file contains hidden or 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-llvm-transforms @llvm/pr-subscribers-llvm-ir Author: Orlando Cazalet-Hyams (OCHyams) ChangesBased on top of #97705, #97719 and #97738. Ignore the first 3 commits, which are under review over there. Patch is 60.56 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/97750.diff 10 Files Affected:
diff --git a/llvm/include/llvm/IR/DbgVariableFragmentInfo.h b/llvm/include/llvm/IR/DbgVariableFragmentInfo.h
new file mode 100644
index 00000000000000..40326d5792f9f9
--- /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 524945862e8d42..e50ff1b9f286ca 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 ed8081a3cad197..fd93dd9b89f7ca 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 7f1489ebbd740c..d86e7c7da2d63f 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 161a30dfb38288..5f0f620684b182 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 locat...
[truncated]
|
Fixes llvm#61981 by adjusting variable location offsets (in the DIExpression) when splittign allocas. AKA Fix debug locations for C++ structured bindings in SROA. NOTE: There's still a bug in mem2reg which generates incorrect locations in some situations: if the variable fragment has an offset into the new (split) alloca, mem2reg will fail to convert that into a bit shift (the location contains a garbage offset). That's not addressed here. insertNewDbgInst - Now takes the address-expression and FragmentInfo as serperate parameters because unlike dbg_declares dbg_assigns want those to go to different places. dbg_assign records put the variable fragment info in the value expression only (whereas dbg_declare has only one expression so puts it there - ideally this information wouldn't live in DIExpression, but that's another issue). MigrateOne - Modified to correctly compute the necessary offsets and fragment adjustments. The previous implementation produced bogus locations for variables with non-zero offsets. The changes replace most of the body of this lambda, so it might be easier to review in a split-diff view and focus on the change as a whole than to compare it to the old implementation. This uses calculateFragmentIntersect and extractLeadingOffset added in previous patches in this series, and createOrReplaceFragment described below. createOrReplaceFragment - Similar to DIExpression::createFragmentExpression except for 3 important distinctions: 1. The new fragment isn't relative to an existing fragment. 2. There are no checks on the the operation types because it is assumed the location this expression is computing is not implicit (i.e., it's always safe to create a fragment because arithmetic operations apply to the address computation, not to an implicit value computation). 3. Existing extract_bits are modified independetly of fragment changes using \p BitExtractOffset. A change to the fragment offset or size may affect a bit extract. But a bit extract offset can change independently of the fragment dimensions. Returns the new expression, or nullptr if one couldn't be created. Ideally this is only used to signal that a bit-extract has become zero-sized (and thus the new debug record has no size and can be dropped), however, it fails for other reasons too - see the FIXME below. FIXME: To keep this patch NFC the function bails in situations that DIExpression::createFragmentExpression fails in.E.g. when fragment and bit extract sizes differ. These limitations can be removed in the future.
OCHyams
force-pushed
the
structured-bindings-fix-4
branch
from
9fc4786 to
da36d79
Compare
jmorse
reviewed
Jul 15, 2024
There was a problem hiding this comment.
Some nits inline; I greatly appreciate that this upgrades the existing implementation from "here's a bucket of bits we swill around" into something that's reasoned and explained with comments and names. The sheer number of dimensions here is mind boggling.
Do we need tests for the composition of fragments and bit extracts? There's code here dealing with bit-extract stuff, but no extra testing for it, are there other tests out there which cover these code paths? (Didn't look at the reviews adding bit-extract stuff as I was in outer woop woop sorry).
| if (const auto *DAI = dyn_cast<DbgAssignIntrinsic>(DVI)) | ||
| return DAI->getAddress(); | ||
| return cast<DbgDeclareInst>(DVI)->getAddress(); | ||
| } |
There was a problem hiding this comment.
I believe the style guide says you need a clear line between function definitions
llvm/lib/Transforms/Scalar/SROA.cpp
Outdated
Comment on lines
5011
to
5014
| /// 2. There are no checks on the the operation types because it is assumed | ||
| /// the location this expression is computing is not implicit (i.e., it's | ||
| /// always safe to create a fragment because arithmetic operations apply | ||
| /// to the address computation, not to an implicit value computation). |
There was a problem hiding this comment.
I feel this comment is difficult to interpret; just state the assumption and say it will fail if given an implicit expression?
There was a problem hiding this comment.
Sorry yep that was a jumble. Is this better? If not I'm happy to trim it down to your suggestion.
llvm/lib/Transforms/Scalar/SROA.cpp
Outdated
| /// the location this expression is computing is not implicit (i.e., it's | ||
| /// always safe to create a fragment because arithmetic operations apply | ||
| /// to the address computation, not to an implicit value computation). | ||
| /// 3. Existing extract_bits are modified independetly of fragment changes |
There was a problem hiding this comment.
Suggested change
| /// 3. Existing extract_bits are modified independetly of fragment changes | |
| /// 3. Existing extract_bits are modified independently of fragment changes |
llvm/lib/Transforms/Scalar/SROA.cpp
Outdated
Comment on lines
5008
to
5009
| /// Similar to DIExpression::createFragmentExpression except for 3 important | ||
| /// distinctions: |
There was a problem hiding this comment.
Should start with a description of what it does do, then follow up with qualifiers
There was a problem hiding this comment.
How does this change look?
| insertNewDbgInst(DIBuilder &DIB, DbgAssignIntrinsic *Orig, AllocaInst *NewAddr, | ||
| DIExpression *NewAddrExpr, Instruction *BeforeInst, | ||
| std::optional<DIExpression::FragmentInfo> NewFragment, | ||
| int64_t BitExtractAdjustment) { | ||
| (void)BeforeInst; |
There was a problem hiding this comment.
Comment explaining this will educate readers (it's because the dbg.assign dictates its own position?)
There was a problem hiding this comment.
This hasn't changed in the patch, but probably should have had a comment on it originally so I've added it now.
llvm/lib/Transforms/Scalar/SROA.cpp
Outdated
Comment on lines
5113
to
5114
| // Migrate debug information from the old alloca to the new alloca(s) | ||
| // and the individual partitions. |
There was a problem hiding this comment.
It's repeated at the lambda call site at the end of this function. Happy to re-instate or move it if you still think it's needed here?
There was a problem hiding this comment.
To my mind a reader going through the function top-to-bottom will want to know what this lambda is doing, hence it's good to have a comment where it's defined. This is a style thing, YMMV.
|
|
||
| ;; Hand-written part to test what happens when variables are smaller than the | ||
| ;; new alloca slices (i.e., check offset rewriting works correctly). Note that | ||
| ;; mem2reg incorrectly preserves the offest in the DIExpression a variable |
There was a problem hiding this comment.
DIExpression a variable -> DIExpression of a variable, doesn't parse for me otherwise?
Comment on lines
132
to
134
| ;; stuffed into the upper bits of a value (that is a bug), e.g. alloca+offset | ||
| ;; becomes vreg+offest. It should either convert the offest to a shift, or | ||
| ;; encode the register-bit offest using DW_OP_bit_piece. |
There was a problem hiding this comment.
Is this a limitation that could be overcome with DW_OP_LLVM_extract_bits_blah, not saying that it should be implemented now?
There was a problem hiding this comment.
Absolutely, good catch (the test was written a while back!).
| ;; splitAlloca may need to be update to account for more complex input | ||
| ;; expressions. Search for this file name in SROA.cpp. | ||
| ; COMMON-LABEL: fun4 | ||
| ; COMMON-NOT: llvm.dbg |
There was a problem hiding this comment.
These NOT-filters are going to be ineffective given that we don't emit intrinsics any more; presumably they should be NOT: #dbg?
There was a problem hiding this comment.
(no longer relevant as I've reworked this part of the test)
Comment on lines
166
to
170
| ;; splitAlloca in SROA.cpp only understands expressions with an optional offset | ||
| ;; and optional fragment. Check a dbg.declare with an extra op is dropped. If | ||
| ;; this test fails it indicates the debug info updating code in SROA.cpp | ||
| ;; splitAlloca may need to be update to account for more complex input | ||
| ;; expressions. Search for this file name in SROA.cpp. |
There was a problem hiding this comment.
This says it expects something to be dropped -- I see two #dbg_declare's in the input function, and two #dbg_value's in the output, am I right to think nothing is dropped?
There was a problem hiding this comment.
How odd. Unfortunately I can't remember or reconstruct what I think the test is meant to be checking. I've thrown it out and replaced it with an extract_bits test. How does this look?
OCHyams
commented
Jul 16, 2024
There was a problem hiding this comment.
Some nits inline; I greatly appreciate that this upgrades the existing implementation from "here's a bucket of bits we swill around" into something that's reasoned and explained with comments and names. The sheer number of dimensions here is mind boggling.
Thanks! I'm glad I managed to finally get around to sorting this out.
Do we need tests for the composition of fragments and bit extracts? There's code here dealing with bit-extract stuff, but no extra testing for it, are there other tests out there which cover these code paths? (Didn't look at the reviews adding bit-extract stuff as I was in outer woop woop sorry).
Good point. To maintain NFC-ness the current implementation doesn't add fragments to expressions that contain bit-extracts. I've added some test coverage of offsets+bit-extracts.
And as stated in createOrReplaceFragment we don't support composition if a fragment exists already in the expression - those dbg records are just thrown away (again, to maintain NFCness - that isn't a fundamental restriction). That's tested in llvm/test/DebugInfo/Generic/sroa-extract-bits.ll (not added in this patch).
llvm/lib/Transforms/Scalar/SROA.cpp
Outdated
Comment on lines
5011
to
5014
| /// 2. There are no checks on the the operation types because it is assumed | ||
| /// the location this expression is computing is not implicit (i.e., it's | ||
| /// always safe to create a fragment because arithmetic operations apply | ||
| /// to the address computation, not to an implicit value computation). |
There was a problem hiding this comment.
Sorry yep that was a jumble. Is this better? If not I'm happy to trim it down to your suggestion.
llvm/lib/Transforms/Scalar/SROA.cpp
Outdated
Comment on lines
5008
to
5009
| /// Similar to DIExpression::createFragmentExpression except for 3 important | ||
| /// distinctions: |
There was a problem hiding this comment.
How does this change look?
| insertNewDbgInst(DIBuilder &DIB, DbgAssignIntrinsic *Orig, AllocaInst *NewAddr, | ||
| DIExpression *NewAddrExpr, Instruction *BeforeInst, | ||
| std::optional<DIExpression::FragmentInfo> NewFragment, | ||
| int64_t BitExtractAdjustment) { | ||
| (void)BeforeInst; |
There was a problem hiding this comment.
This hasn't changed in the patch, but probably should have had a comment on it originally so I've added it now.
llvm/lib/Transforms/Scalar/SROA.cpp
Outdated
Comment on lines
5113
to
5114
| // Migrate debug information from the old alloca to the new alloca(s) | ||
| // and the individual partitions. |
There was a problem hiding this comment.
It's repeated at the lambda call site at the end of this function. Happy to re-instate or move it if you still think it's needed here?
Comment on lines
132
to
134
| ;; stuffed into the upper bits of a value (that is a bug), e.g. alloca+offset | ||
| ;; becomes vreg+offest. It should either convert the offest to a shift, or | ||
| ;; encode the register-bit offest using DW_OP_bit_piece. |
There was a problem hiding this comment.
Absolutely, good catch (the test was written a while back!).
Comment on lines
166
to
170
| ;; splitAlloca in SROA.cpp only understands expressions with an optional offset | ||
| ;; and optional fragment. Check a dbg.declare with an extra op is dropped. If | ||
| ;; this test fails it indicates the debug info updating code in SROA.cpp | ||
| ;; splitAlloca may need to be update to account for more complex input | ||
| ;; expressions. Search for this file name in SROA.cpp. |
There was a problem hiding this comment.
How odd. Unfortunately I can't remember or reconstruct what I think the test is meant to be checking. I've thrown it out and replaced it with an extract_bits test. How does this look?
jmorse
approved these changes
Jul 17, 2024
llvm/lib/Transforms/Scalar/SROA.cpp
Outdated
Comment on lines
5113
to
5114
| // Migrate debug information from the old alloca to the new alloca(s) | ||
| // and the individual partitions. |
There was a problem hiding this comment.
To my mind a reader going through the function top-to-bottom will want to know what this lambda is doing, hence it's good to have a comment where it's defined. This is a style thing, YMMV.
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
Summary: Fixes issue #61981 by adjusting variable location offsets (in the DIExpression) when splitting allocas. Patch [4/4] to fix structured bindings in SROA. NOTE: There's still a bug in mem2reg which generates incorrect locations in some situations: if the variable fragment has an offset into the new (split) alloca, mem2reg will fail to convert that into a bit shift (the location contains a garbage offset). That's not addressed here. insertNewDbgInst - Now takes the address-expression and FragmentInfo as separate parameters because unlike dbg_declares dbg_assigns want those to go to different places. dbg_assign records put the variable fragment info in the value expression only (whereas dbg_declare has only one expression so puts it there - ideally this information wouldn't live in DIExpression, but that's another issue). MigrateOne - Modified to correctly compute the necessary offsets and fragment adjustments. The previous implementation produced bogus locations for variables with non-zero offsets. The changes replace most of the body of this lambda, so it might be easier to review in a split-diff view and focus on the change as a whole than to compare it to the old implementation. This uses calculateFragmentIntersect and extractLeadingOffset added in previous patches in this series, and createOrReplaceFragment described below. createOrReplaceFragment - Similar to DIExpression::createFragmentExpression except for 3 important distinctions: 1. The new fragment isn't relative to an existing fragment. 2. There are no checks on the the operation types because it is assumed the location this expression is computing is not implicit (i.e., it's always safe to create a fragment because arithmetic operations apply to the address computation, not to an implicit value computation). 3. Existing extract_bits are modified independetly of fragment changes using \p BitExtractOffset. A change to the fragment offset or size may affect a bit extract. But a bit extract offset can change independently of the fragment dimensions. Returns the new expression, or nullptr if one couldn't be created. Ideally this is only used to signal that a bit-extract has become zero-sized (and thus the new debug record has no size and can be dropped), however, it fails for other reasons too - see the FIXME below. FIXME: To keep the scope of this change focused on non-bitfield structured bindings the function bails in situations that DIExpression::createFragmentExpression fails. E.g. when fragment and bit extract sizes differ. These limitations can be removed in the future. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250875
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.
Based on top of #97705, #97719 and #97738.
Ignore the first 3 commits, which are under review over there(rebased).