[compiler-rt] c971f98 - tsan: de-hardcode number of unused bits in trace events

[Dmitry Vyukov via llvm-commits]

Author: Dmitry Vyukov
Date: 2021-11-16T16:00:14+01:00
New Revision: c971f989ee9e1356308933e809ecbbbc5976c581

URL: https://github.com/llvm/llvm-project/commit/c971f989ee9e1356308933e809ecbbbc5976c581
DIFF: https://github.com/llvm/llvm-project/commit/c971f989ee9e1356308933e809ecbbbc5976c581.diff

LOG: tsan: de-hardcode number of unused bits in trace events

Precisely specifying the unused parts of the bitfield is critical for
performance. If we don't specify them, compiler will generate code to load
the old value and shuffle it to extract the unused bits to apply to the new
value. If we specify the unused part and store 0 in there, all that
unnecessary code goes away (store of the 0 const is combined with other
constant parts).

I don't see a good way to ensure we cover all of u64 bits with fields.
So at least introduce named kUnusedBits consts and check that bits
sum up to 64.

Depends on D113978.

Reviewed By: melver

Differential Revision: https://reviews.llvm.org/D113979

Added: 
    

Modified: 
    compiler-rt/lib/tsan/rtl/tsan_trace.h

Removed: 
    


################################################################################
diff  --git a/compiler-rt/lib/tsan/rtl/tsan_trace.h b/compiler-rt/lib/tsan/rtl/tsan_trace.h
index c13fb12e1b90..ffc8c991ece0 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_trace.h
+++ b/compiler-rt/lib/tsan/rtl/tsan_trace.h
@@ -99,6 +99,8 @@ static constexpr Event NopEvent = {1, 0, EventType::kAccessExt, 0};
 // close enough to each other. Otherwise we fall back to EventAccessExt.
 struct EventAccess {
   static constexpr uptr kPCBits = 15;
+  static_assert(kPCBits + kCompressedAddrBits + 5 == 64,
+                "unused bits in EventAccess");
 
   u64 is_access : 1;  // = 1
   u64 is_read : 1;
@@ -119,13 +121,23 @@ static_assert(sizeof(EventFunc) == 8, "bad EventFunc size");
 
 // Extended memory access with full PC.
 struct EventAccessExt {
+  // Note: precisely specifying the unused parts of the bitfield is critical for
+  // performance. If we don't specify them, compiler will generate code to load
+  // the old value and shuffle it to extract the unused bits to apply to the new
+  // value. If we specify the unused part and store 0 in there, all that
+  // unnecessary code goes away (store of the 0 const is combined with other
+  // constant parts).
+  static constexpr uptr kUnusedBits = 11;
+  static_assert(kCompressedAddrBits + kUnusedBits + 9 == 64,
+                "unused bits in EventAccessExt");
+
   u64 is_access : 1;   // = 0
   u64 is_func : 1;     // = 0
   EventType type : 3;  // = EventType::kAccessExt
   u64 is_read : 1;
   u64 is_atomic : 1;
   u64 size_log : 2;
-  u64 _ : 11;
+  u64 _ : kUnusedBits;
   u64 addr : kCompressedAddrBits;
   u64 pc;
 };
@@ -134,6 +146,8 @@ static_assert(sizeof(EventAccessExt) == 16, "bad EventAccessExt size");
 // Access to a memory range.
 struct EventAccessRange {
   static constexpr uptr kSizeLoBits = 13;
+  static_assert(kCompressedAddrBits + kSizeLoBits + 7 == 64,
+                "unused bits in EventAccessRange");
 
   u64 is_access : 1;   // = 0
   u64 is_func : 1;     // = 0
@@ -150,6 +164,13 @@ static_assert(sizeof(EventAccessRange) == 16, "bad EventAccessRange size");
 // Mutex lock.
 struct EventLock {
   static constexpr uptr kStackIDLoBits = 15;
+  static constexpr uptr kStackIDHiBits =
+      sizeof(StackID) * kByteBits - kStackIDLoBits;
+  static constexpr uptr kUnusedBits = 3;
+  static_assert(kCompressedAddrBits + kStackIDLoBits + 5 == 64,
+                "unused bits in EventLock");
+  static_assert(kCompressedAddrBits + kStackIDHiBits + kUnusedBits == 64,
+                "unused bits in EventLock");
 
   u64 is_access : 1;   // = 0
   u64 is_func : 1;     // = 0
@@ -157,29 +178,37 @@ struct EventLock {
   u64 pc : kCompressedAddrBits;
   u64 stack_lo : kStackIDLoBits;
   u64 stack_hi : sizeof(StackID) * kByteBits - kStackIDLoBits;
-  u64 _ : 3;
+  u64 _ : kUnusedBits;
   u64 addr : kCompressedAddrBits;
 };
 static_assert(sizeof(EventLock) == 16, "bad EventLock size");
 
 // Mutex unlock.
 struct EventUnlock {
+  static constexpr uptr kUnusedBits = 15;
+  static_assert(kCompressedAddrBits + kUnusedBits + 5 == 64,
+                "unused bits in EventUnlock");
+
   u64 is_access : 1;   // = 0
   u64 is_func : 1;     // = 0
   EventType type : 3;  // = EventType::kUnlock
-  u64 _ : 15;
+  u64 _ : kUnusedBits;
   u64 addr : kCompressedAddrBits;
 };
 static_assert(sizeof(EventUnlock) == 8, "bad EventUnlock size");
 
 // Time change event.
 struct EventTime {
+  static constexpr uptr kUnusedBits = 37;
+  static_assert(kUnusedBits + sizeof(Sid) * kByteBits + kEpochBits + 5 == 64,
+                "unused bits in EventTime");
+
   u64 is_access : 1;   // = 0
   u64 is_func : 1;     // = 0
   EventType type : 3;  // = EventType::kTime
   u64 sid : sizeof(Sid) * kByteBits;
   u64 epoch : kEpochBits;
-  u64 _ : 64 - 5 - sizeof(Sid) * kByteBits - kEpochBits;
+  u64 _ : kUnusedBits;
 };
 static_assert(sizeof(EventTime) == 8, "bad EventTime size");