[liftoff] Selectively use scoped code comments

For bigger code blocks it makes sense to use the "scoped comments" with
an opening and closing bracket. In particular, if more such scoped
comments are already generated inside the block (e.g. for decoding the
sandboxed pointer, or for AssertZeroExtended).

Thus add scoped comments around loading from memory and storing to
memory.

Drive-by: Mark the {CodeComment} constructor V8_NODISCARD so we do not
accidentally define a temporary object that dies right away.
Drive-by 2: Remove the "#undef"s at the end of liftoff-compiler.cc; we
do not support jumbo builds any more anyway.

R=thibaudm@chromium.org

Change-Id: If4af8e9f4288529e0fe176c7f0f8376474cfa469
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4096737
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/main@{#84876}

src/codegen/assembler.h

@@ -323,7 +323,7 @@ class V8_EXPORT_PRIVATE AssemblerBase : public Malloced {
 #ifdef V8_CODE_COMMENTS
   class CodeComment {
    public:
-    explicit CodeComment(Assembler* assembler, const std::string& comment)
+    V8_NODISCARD CodeComment(Assembler* assembler, const std::string& comment)
         : assembler_(assembler) {
       if (v8_flags.code_comments) Open(comment);
     }
@@ -340,7 +340,7 @@ class V8_EXPORT_PRIVATE AssemblerBase : public Malloced {
   };
 #else  // V8_CODE_COMMENTS
   class CodeComment {
-    explicit CodeComment(Assembler* assembler, std::string comment) {}
+    V8_NODISCARD CodeComment(Assembler*, const std::string&) {}
   };
 #endif
 

src/wasm/baseline/liftoff-compiler.cc

@@ -34,9 +34,7 @@
 #include "src/wasm/wasm-objects.h"
 #include "src/wasm/wasm-opcodes-inl.h"
 
-namespace v8 {
+namespace v8::internal::wasm {
-namespace internal {
-namespace wasm {
 
 constexpr auto kRegister = LiftoffAssembler::VarState::kRegister;
 constexpr auto kIntConst = LiftoffAssembler::VarState::kIntConst;
@@ -86,12 +84,12 @@ struct assert_field_size {
                                    WASM_INSTANCE_OBJECT_FIELD_OFFSET(name));
 
 #ifdef V8_CODE_COMMENTS
-#define CODE_COMMENT(str)  \
+#define CODE_COMMENT(str) __ RecordComment(str)
-  do {                     \
+#define SCOPED_CODE_COMMENT(str) \
-    __ RecordComment(str); \
+  AssemblerBase::CodeComment scoped_comment_##__LINE__(&asm_, str)
-  } while (false)
 #else
 #define CODE_COMMENT(str) ((void)0)
+#define SCOPED_CODE_COMMENT(str) ((void)0)
 #endif
 
 constexpr LoadType::LoadTypeValue kPointerLoadType =
@@ -340,7 +338,6 @@ void CheckBailoutAllowed(LiftoffBailoutReason reason, const char* detail,
 class LiftoffCompiler {
  public:
   using ValidationTag = Decoder::BooleanValidationTag;
-
   using Value = ValueBase<ValidationTag>;
 
   struct ElseState {
@@ -3140,6 +3137,7 @@ class LiftoffCompiler {
   Register GetMemoryStart(LiftoffRegList pinned) {
     Register memory_start = __ cache_state()->cached_mem_start;
     if (memory_start == no_reg) {
+      SCOPED_CODE_COMMENT("load memory start");
       memory_start = __ GetUnusedRegister(kGpReg, pinned).gp();
       LOAD_INSTANCE_FIELD(memory_start, MemoryStart, kSystemPointerSize,
                           pinned);
@@ -3170,7 +3168,7 @@ class LiftoffCompiler {
     bool i64_offset = index_slot.kind() == kI64;
     if (IndexStaticallyInBounds(index_slot, type.size(), &offset)) {
       __ cache_state()->stack_state.pop_back();
-      CODE_COMMENT("load from memory (constant offset)");
+      SCOPED_CODE_COMMENT("load from memory (constant offset)");
       LiftoffRegList pinned;
       Register mem = pinned.set(GetMemoryStart(pinned));
       LiftoffRegister value = pinned.set(__ GetUnusedRegister(rc, pinned));
@@ -3182,7 +3180,7 @@ class LiftoffCompiler {
                              kDontForceCheck);
       if (index == no_reg) return;
 
-      CODE_COMMENT("load from memory");
+      SCOPED_CODE_COMMENT("load from memory");
       LiftoffRegList pinned{index};
 
       // Load the memory start address only now to reduce register pressure
@@ -3306,7 +3304,7 @@ class LiftoffCompiler {
     bool i64_offset = index_slot.kind() == kI64;
     if (IndexStaticallyInBounds(index_slot, type.size(), &offset)) {
       __ cache_state()->stack_state.pop_back();
-      CODE_COMMENT("store to memory (constant offset)");
+      SCOPED_CODE_COMMENT("store to memory (constant offset)");
       Register mem = pinned.set(GetMemoryStart(pinned));
       __ Store(mem, no_reg, offset, value, type, pinned, nullptr, true,
                i64_offset);
@@ -3317,7 +3315,7 @@ class LiftoffCompiler {
       if (index == no_reg) return;
 
       pinned.set(index);
-      CODE_COMMENT("store to memory");
+      SCOPED_CODE_COMMENT("store to memory");
       uint32_t protected_store_pc = 0;
       // Load the memory start address only now to reduce register pressure
       // (important on ia32).
@@ -7771,6 +7769,7 @@ class LiftoffCompiler {
   Register LoadInstanceIntoRegister(LiftoffRegList pinned, Register fallback) {
     Register instance = __ cache_state()->cached_instance;
     if (instance == no_reg) {
+      SCOPED_CODE_COMMENT("load instance");
       instance = __ cache_state()->TrySetCachedInstanceRegister(
           pinned | LiftoffRegList{fallback});
       if (instance == no_reg) instance = fallback;
@@ -7991,14 +7990,4 @@ std::unique_ptr<DebugSideTable> GenerateLiftoffDebugSideTable(
   return debug_sidetable_builder.GenerateDebugSideTable();
 }
 
-#undef __
+}  // namespace v8::internal::wasm
-#undef TRACE
-#undef WASM_INSTANCE_OBJECT_FIELD_OFFSET
-#undef WASM_INSTANCE_OBJECT_FIELD_SIZE
-#undef LOAD_INSTANCE_FIELD
-#undef LOAD_TAGGED_PTR_INSTANCE_FIELD
-#undef CODE_COMMENT
-
-}  // namespace wasm
-}  // namespace internal
-}  // namespace v8