[InstrProf] Change step from 64-bit to pointer-sized int

[jdmitrovic-syrmia]

Fixed 64-bit step can lead to creating atomic instructions unsupported by the target architecture (see rust-lang/rust#112313).

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-pgo
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: Jovan Dmitrović (jdmitrovic-syrmia)

Changes

Fixed 64-bit step can lead to creating atomic instructions unsupported by the target architecture (see rust-lang/rust#112313).

---

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

1 Files Affected:

• (modified) llvm/lib/IR/IntrinsicInst.cpp (+2-1)

diff --git a/llvm/lib/IR/IntrinsicInst.cpp b/llvm/lib/IR/IntrinsicInst.cpp
index 89403e1d7fcb4d..0f0a6bf23ffe43 100644
--- a/llvm/lib/IR/IntrinsicInst.cpp
+++ b/llvm/lib/IR/IntrinsicInst.cpp
@@ -288,7 +288,8 @@ Value *InstrProfIncrementInst::getStep() const {
   }
   const Module *M = getModule();
   LLVMContext &Context = M->getContext();
-  return ConstantInt::get(Type::getInt64Ty(Context), 1);
+  const auto &DL = M->getDataLayout();
+  return ConstantInt::get(DL.getIntPtrType(Context), 1);
 }
 
 std::optional<RoundingMode> ConstrainedFPIntrinsic::getRoundingMode() const {

[briansmith]

With a 64-bit type, very few counters will overflow. With a pointer-width type, more counters will overflow, especially on 16-bit targets, but even on 32-bit targets. I speculate this is why 64-bit integers are used currently. How should we address this concern?

[jdmitrovic-syrmia]

I see. I pushed some changes, creating additional getStep function for atomics. This way, we can use a larger type.

Also, I've taken the liberty of changing the existing code to use the largest legal int available instead of the 64-bit int.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[briansmith]

@jdmitrovic-syrmia I would love to review this but I am not an LLVM project member and I don't understand the implementation well enough to offer useful feedback on the implementation.

For my interests, I am primarily interested in code coverage and not profiling.

I also found https://discourse.llvm.org/t/rfc-single-byte-counters-for-source-based-code-coverage/75685 which addresses the problem in a different way. Note that it is claimed there that "A nice advantage of single-byte counters is that it is already thread-safe without needing atomics." It was just merged a couple of weeks ago; see #75425.

[jdmitrovic-syrmia]

@briansmith It seems to me that #75425 concerns code coverage, and not profiling. I don't think this PR is affecting the code coverage analysis. Also, #75425 doesn't resolve the Rust issue I mentioned.

[MaskRay]

This is incorrect. See rust-lang/rust#112313
In the absence of __sync_fetch_and_add_8, the best is to consider the feature unusable on Rust's mips port.

[briansmith]

@jdmitrovic-syrmia As an alternative solution, why not implement a (perhaps slow) atomic 64-bit fallback implementation that uses a global lock?