[Reassociate] Use a reference to DataLayout instead of copying the underlying string data (NFC)

[cooperp]

I noticed this when looking at all allocations by clang. For a medium sized file this was around 6000 calls to operator new, although i suspect there were more allocations in total as the SmallVectors in DataLayout may have their own allocations in some cases.

In a follow-up i'm tempted to make the DataLayout copy constructor private, to avoid this in future. There are a few tests which copy the DataLayout, and perhaps need to (I didn't check yet), but we could provide a clone() method for them if needed. Its only accidental copying I think we should consider avoiding, not people who really do need to copy it for reasons.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: None (cooperp)

Changes

I noticed this when looking at all allocations by clang. For a medium sized file this was around 6000 calls to operator new, although i suspect there were more allocations in total as the SmallVectors in DataLayout may have their own allocations in some cases.

In a follow-up i'm tempted to make the DataLayout copy constructor private, to avoid this in future. There are a few tests which copy the DataLayout, and perhaps need to (I didn't check yet), but we could provide a clone() method for them if needed. Its only accidental copying I think we should consider avoiding, not people who really do need to copy it for reasons.

---

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

1 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/Reassociate.cpp (+1-1)

diff --git a/llvm/lib/Transforms/Scalar/Reassociate.cpp b/llvm/lib/Transforms/Scalar/Reassociate.cpp
index 7cb9bace47bf4..d3476ab992269 100644
--- a/llvm/lib/Transforms/Scalar/Reassociate.cpp
+++ b/llvm/lib/Transforms/Scalar/Reassociate.cpp
@@ -420,7 +420,7 @@ static bool LinearizeExprTree(Instruction *I,
   using LeafMap = DenseMap<Value *, uint64_t>;
   LeafMap Leaves; // Leaf -> Total weight so far.
   SmallVector<Value *, 8> LeafOrder; // Ensure deterministic leaf output order.
-  const DataLayout DL = I->getDataLayout();
+  const DataLayout& DL = I->getDataLayout();
 
 #ifndef NDEBUG
   SmallPtrSet<Value *, 8> Visited; // For checking the iteration scheme.

[nikic]

In a follow-up i'm tempted to make the DataLayout copy constructor private, to avoid this in future. There are a few tests which copy the DataLayout, and perhaps need to (I didn't check yet), but we could provide a clone() method for them if needed. Its only accidental copying I think we should consider avoiding, not people who really do need to copy it for reasons.

Maybe making the copy constructor explicit would be good enough?