[SROA] Unify the names of new instructions created in SROA. #167917
Conversation
In Debug builds, the names of adjusted pointers have a pointer-specific name prefix which doesn't exist in non-debug builds. This causes differences in output when looking at the output of SROA with a Debug or Release compiler. For most of our ongoing testing, we use essentially Release+Asserts build (basically release but without NDEBUG defined), however we ship a Release compiler. Therefore we want to say with reasonable confidence that building a large project with Release vs a Release+Asserts build gives us the same output when the same compiler version is used. This difference however, makes it difficult to prove that the output is the same if the only difference is the name when using LTO builds and looking at bitcode. Hence this change is being proposed.
|
To add to this, I looked at the output of the compile-time tracker and it looks to not increase compile-times by any significant amount |
|
@llvm/pr-subscribers-llvm-transforms Author: Shubham Sandeep Rastogi (rastogishubham) ChangesIn Debug builds, the names of adjusted pointers have a pointer-specific name prefix which doesn't exist in non-debug builds. This causes differences in output when looking at the output of SROA with a Debug or Release compiler. For most of our ongoing testing, we use essentially Release+Asserts build (basically release but without NDEBUG defined), however we ship a Release compiler. Therefore we want to say with reasonable confidence that building a large project with Release vs a Release+Asserts build gives us the same output when the same compiler version is used. This difference however, makes it difficult to prove that the output is the same if the only difference is the name when using LTO builds and looking at bitcode. Hence this change is being proposed. Full diff: https://github.com/llvm/llvm-project/pull/167917.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/SROA.cpp b/llvm/lib/Transforms/Scalar/SROA.cpp
index 5c60fad6f91aa..70afe833c9f47 100644
--- a/llvm/lib/Transforms/Scalar/SROA.cpp
+++ b/llvm/lib/Transforms/Scalar/SROA.cpp
@@ -3150,7 +3150,6 @@ class AllocaSliceRewriter : public InstVisitor<AllocaSliceRewriter, bool> {
assert(IsSplit || BeginOffset == NewBeginOffset);
uint64_t Offset = NewBeginOffset - NewAllocaBeginOffset;
-#ifndef NDEBUG
StringRef OldName = OldPtr->getName();
// Skip through the last '.sroa.' component of the name.
size_t LastSROAPrefix = OldName.rfind(".sroa.");
@@ -3169,17 +3168,10 @@ class AllocaSliceRewriter : public InstVisitor<AllocaSliceRewriter, bool> {
}
// Strip any SROA suffixes as well.
OldName = OldName.substr(0, OldName.find(".sroa_"));
-#endif
return getAdjustedPtr(IRB, DL, &NewAI,
APInt(DL.getIndexTypeSizeInBits(PointerTy), Offset),
- PointerTy,
-#ifndef NDEBUG
- Twine(OldName) + "."
-#else
- Twine()
-#endif
- );
+ PointerTy, Twine(OldName) + ".");
}
/// Compute suitable alignment to access this slice of the *new*
|
|
instruction names don't affect compilation, so the motivation doesn't really make sense |
|
@aeubanks Please correct me if I am wrong, if we do an LTO build and the names are different because one LTO build is Release + Asserts without NDEBUG defined, and the other is with a Release compiler (where NDEBUG) would be defined, wouldn't the bitcode be different, because NDEBUG is what causes the names to be different? |
|
This is important for us at Sony because we have tooling that analyzes bitcode to prove equivalence as a means to reduce testing by eliminating redundant test sets across different configurations. The change @rastogishubham is proposing will help eliminate a difference that doesn't seem necessary. |
|
@aeubanks just pinging again |
4 participants
In Debug builds, the names of adjusted pointers have a pointer-specific name prefix which doesn't exist in non-debug builds.
This causes differences in output when looking at the output of SROA with a Debug or Release compiler.
For most of our ongoing testing, we use essentially Release+Asserts build (basically release but without NDEBUG defined), however we ship a Release compiler. Therefore we want to say with reasonable confidence that building a large project with Release vs a Release+Asserts build gives us the same output when the same compiler version is used.
This difference however, makes it difficult to prove that the output is the same if the only difference is the name when using LTO builds and looking at bitcode.
Hence this change is being proposed.