Skip to content
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

[MemProf][NFC] remove unneeded TypeSize in InterestingMemoryAccess #79244

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

Enna1
Copy link
Contributor

@Enna1 Enna1 commented Jan 24, 2024

Unlike ASan, MemProf uses the same memory access callback(inline sequence) for different size memory access, remove unneeded TypeSize stored in InterestingMemoryAccess.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 24, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Enna1 (Enna1)

Changes

Unlike ASan, MemProf uses the same memory access callback(inline sequence) for different size memory access, remove unneeded TypeSize stored in InterestingMemoryAccess.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/Instrumentation/MemProfiler.cpp (+4-8)
diff --git a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
index 2236e9cd44c504..57a89350a47b34 100644
--- a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
@@ -171,7 +171,6 @@ struct InterestingMemoryAccess {
   Value *Addr = nullptr;
   bool IsWrite;
   Type *AccessTy;
-  uint64_t TypeSize;
   Value *MaybeMask = nullptr;
 };
 
@@ -194,7 +193,7 @@ class MemProfiler {
   void instrumentMop(Instruction *I, const DataLayout &DL,
                      InterestingMemoryAccess &Access);
   void instrumentAddress(Instruction *OrigIns, Instruction *InsertBefore,
-                         Value *Addr, uint32_t TypeSize, bool IsWrite);
+                         Value *Addr, bool IsWrite);
   void instrumentMaskedLoadOrStore(const DataLayout &DL, Value *Mask,
                                    Instruction *I, Value *Addr, Type *AccessTy,
                                    bool IsWrite);
@@ -375,7 +374,6 @@ MemProfiler::isInterestingMemoryAccess(Instruction *I) const {
   }
 
   const DataLayout &DL = I->getModule()->getDataLayout();
-  Access.TypeSize = DL.getTypeStoreSizeInBits(Access.AccessTy);
   return Access;
 }
 
@@ -383,7 +381,6 @@ void MemProfiler::instrumentMaskedLoadOrStore(const DataLayout &DL, Value *Mask,
                                               Instruction *I, Value *Addr,
                                               Type *AccessTy, bool IsWrite) {
   auto *VTy = cast<FixedVectorType>(AccessTy);
-  uint64_t ElemTypeSize = DL.getTypeStoreSizeInBits(VTy->getScalarType());
   unsigned Num = VTy->getNumElements();
   auto *Zero = ConstantInt::get(IntptrTy, 0);
   for (unsigned Idx = 0; Idx < Num; ++Idx) {
@@ -408,8 +405,7 @@ void MemProfiler::instrumentMaskedLoadOrStore(const DataLayout &DL, Value *Mask,
     IRBuilder<> IRB(InsertBefore);
     InstrumentedAddress =
         IRB.CreateGEP(VTy, Addr, {Zero, ConstantInt::get(IntptrTy, Idx)});
-    instrumentAddress(I, InsertBefore, InstrumentedAddress, ElemTypeSize,
-                      IsWrite);
+    instrumentAddress(I, InsertBefore, InstrumentedAddress, IsWrite);
   }
 }
 
@@ -436,13 +432,13 @@ void MemProfiler::instrumentMop(Instruction *I, const DataLayout &DL,
     // Since the access counts will be accumulated across the entire allocation,
     // we only update the shadow access count for the first location and thus
     // don't need to worry about alignment and type size.
-    instrumentAddress(I, I, Access.Addr, Access.TypeSize, Access.IsWrite);
+    instrumentAddress(I, I, Access.Addr, Access.IsWrite);
   }
 }
 
 void MemProfiler::instrumentAddress(Instruction *OrigIns,
                                     Instruction *InsertBefore, Value *Addr,
-                                    uint32_t TypeSize, bool IsWrite) {
+                                    bool IsWrite) {
   IRBuilder<> IRB(InsertBefore);
   Value *AddrLong = IRB.CreatePointerCast(Addr, IntptrTy);
 

@Enna1
Copy link
Contributor Author

Enna1 commented Jan 24, 2024

BTW, we also add declarations for __memprof_loadN and __memprof_storeN in instrumentation, but not implement them in runtime.
Not sure if we should implement them in memprof_interface_internal.h or just remove the declarations for __memprof_loadN and __memprof_storeN when do instrumentation.

@teresajohnson
Copy link
Contributor

BTW, we also add declarations for __memprof_loadN and __memprof_storeN in instrumentation, but not implement them in runtime. Not sure if we should implement them in memprof_interface_internal.h or just remove the declarations for __memprof_loadN and __memprof_storeN when do instrumentation.

Yeah this looks like leftover stuff carried over from Asan. I think that can also be removed from the instrumentation code. Thanks for the cleanup!

Enna1 added a commit that referenced this pull request Jan 24, 2024
As discussed in #79244,
the sized memory access callback is leftover stuff carried over from Asan,
can removed from the instrumentation.
Enna1 added a commit that referenced this pull request Jan 25, 2024
As discussed in #79244,
the sized memory access callback is leftover stuff carried over from Asan,
can removed from the instrumentation.
@Enna1 Enna1 force-pushed the users/Enna1/memprof-unneeded-typesize branch from c4698cc to 8d1ede0 Compare January 25, 2024 00:25
@Enna1 Enna1 merged commit a739589 into main Jan 25, 2024
3 of 4 checks passed
@Enna1 Enna1 deleted the users/Enna1/memprof-unneeded-typesize branch January 25, 2024 02:01
Enna1 added a commit that referenced this pull request Jan 25, 2024
As discussed in #79244, the
sized memory access callback is leftover stuff carried over from Asan,
can removed from the instrumentation.
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