[llvm] InstProfiling: Give names to profc_bias and profc_addr. NFC. (PR #95587)

NAKAMURA Takumi via llvm-commits llvm-commits at lists.llvm.org
Sat Jun 15 18:50:35 PDT 2024


================
@@ -926,10 +926,10 @@ Value *InstrLowerer::getCounterAddress(InstrProfCntrInstBase *I) {
       if (TT.supportsCOMDAT())
         Bias->setComdat(M.getOrInsertComdat(Bias->getName()));
     }
-    BiasLI = EntryBuilder.CreateLoad(Int64Ty, Bias);
+    BiasLI = EntryBuilder.CreateLoad(Int64Ty, Bias, "profc_bias");
   }
   auto *Add = Builder.CreateAdd(Builder.CreatePtrToInt(Addr, Int64Ty), BiasLI);
-  return Builder.CreateIntToPtr(Add, Addr->getType());
+  return Builder.CreateIntToPtr(Add, Addr->getType(), "profc_addr");
----------------
chapuni wrote:

Thanks to suggest a consideration. Let me speak my justifications.

Generally, this is just for instrumentation mode. So, we could consider how much compilation time would be impacted in instrumentation modes. Named Insts will have extra cost (ex. adding suffixes) when they (Insts) are copied or propagated in transformations. In contrast, Insts' users won't have any costs since using just refers to Insts. I haven't measured yet.

Re. `profc_bias`, I can justify. It will be merged to the single Inst with optimizations. (#95588)
The named Value will help readability for looking into larger funcs.

Re. `profc_addr`, it is referred in two places with load/store, in one place with atomic rmw. (will become two places when TATAS is introduced) I guess it won't enhance readability so much, since the scope of `profc_addr` is small. That said, I think it's good thing to mark key Values (Insts) named..

An example from #95588, with optimized

With names;
```
define void @foo() {
  %profc_bias = load i64, ptr @__llvm_profile_counter_bias, align 8, !invariant.load !0
  %1 = add i64 ptrtoint (ptr @__profc_foo to i64), %profc_bias
  %profc_addr = inttoptr i64 %1 to ptr
  %pgocount = load i64, ptr %profc_addr, align 8
  %2 = add i64 %pgocount, 1
  store i64 %2, ptr %profc_addr, align 8
  %3 = add i64 ptrtoint (ptr @__profc_bar to i64), %profc_bias
  %profc_addr.i = inttoptr i64 %3 to ptr
  %pgocount.i = load i64, ptr %profc_addr.i, align 8
  %4 = add i64 %pgocount.i, 1
  store i64 %4, ptr %profc_addr.i, align 8
  ret void
}
```

W/o names;
```
define void @foo() {
  %1 = load i64, ptr @__llvm_profile_counter_bias, align 8, !invariant.load !0
  %2 = add i64 ptrtoint (ptr @__profc_foo to i64), %1
  %3 = inttoptr i64 %2 to ptr
  %pgocount = load i64, ptr %3, align 8
  %4 = add i64 %pgocount, 1
  store i64 %4, ptr %3, align 8
  %5 = add i64 ptrtoint (ptr @__profc_bar to i64), %1
  %6 = inttoptr i64 %5 to ptr
  %pgocount.i = load i64, ptr %6, align 8
  %7 = add i64 %pgocount.i, 1
  store i64 %7, ptr %6, align 8
  ret void
}
```

Could we suppose it would justify the microcost for naming a few Insts per region?

https://github.com/llvm/llvm-project/pull/95587


More information about the llvm-commits mailing list