Change getTgtMemIntrinsic interface with returning a pair #1

HankChang736 · 2025-08-18T06:26:59Z

This patch change the interface of getTgtMemIntrinsic from return "bool" into return "std::pair<std::optional,std::optional<SmallVectorImpl>>" to allow Asan or EarlyCSE to choose the data structure they need with the same TTI hook.

* This patch change the interface of getTgtMemIntrinsic from return "bool" into return "std::pair<std::optional<MemIntrinsicInfo>,std::optional<SmallVectorImpl<InterestingMemoryOperand>>>" to allow Asan or EarlyCSE to choose the data structure they need with the same TTI hook.

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

llvm/lib/Transforms/Scalar/EarlyCSE.cpp

topperc · 2025-08-19T20:51:56Z

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h

-                                  MemIntrinsicInfo &Info) const {
-    return false;
+  virtual std::pair<std::optional<MemIntrinsicInfo>,
+                    std::optional<SmallVector<InterestingMemoryOperand, 1>>>


Do we need an optional around the SmallVector? Can we use an empty vector to represent no interesting operand?

Looks like EarlyCSE will ignore MemIntrinsicInfo with a null PtrVal. Can we use that instead of an optional?

I think use an empty vector to represent no interesting operand is a great idea, in this way we don't need to use optional for SmallVector.

Use a null PtrVal to check MemIntrinsicInfo might be fine, I just checked the target that uses getTgtMemIntrinsic and find that all of them fill PtrVal and EarlyCSE/LoopStrengthReduce also ignore PtrVal with nullptr.

I will try to modify it with dropping optional.

Suggested change

std::optional<SmallVector<InterestingMemoryOperand, 1>>>

std::pair<MemIntrinsicInfo,

SmallVector<InterestingMemoryOperand, 1>>

The interface after dropping std::optional will look like this.

topperc · 2025-08-19T21:00:48Z

llvm/include/llvm/Analysis/TargetTransformInfo.h

 #include "llvm/Support/BranchProbability.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/InstructionCost.h"
+#include "llvm/Transforms/Instrumentation/AddressSanitizerCommon.h"


I don't think Analysis should include a header from Transforms. The Transforms library has a dependency on the Analysis library.

Oh I see. The reason I include this header from Transforms is to use InterestingMemoryOperand, maybe move InterestingMemoryOperand from Transforms header to Analysis/InterestingMemoryOperand.h and include it in Transforms/Instrumentation/AddressSanitizerCommon.h ?

That sounds good to me.

HankChang736 · 2025-08-20T14:13:30Z

llvm/lib/Transforms/Scalar/EarlyCSE.cpp

+        std::pair<std::optional<MemIntrinsicInfo>,
+                  std::optional<SmallVector<InterestingMemoryOperand, 1>>>
+            MemInfo = TTI.getTgtMemIntrinsic(II);
+        if (MemInfo.first != std::nullopt) {


Suggested change

if (MemInfo.first != std::nullopt) {

if (MemInfo.first.PtrVal) {

Check PtrVal is null or not to see whether the target fill MemIntrinsicInfo.

HankChang736 · 2025-08-20T14:15:13Z

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

+      std::pair<std::optional<MemIntrinsicInfo>,
+                std::optional<SmallVector<InterestingMemoryOperand, 1>>>
+          MemInfo = TTI.getTgtMemIntrinsic(II);
+      if (MemInfo.first != std::nullopt) {


Suggested change

if (MemInfo.first != std::nullopt) {

if (MemInfo.first.PtrVal) {

Same here

HankChang736 · 2025-08-20T14:17:05Z

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

+      std::pair<std::optional<MemIntrinsicInfo>,
+                std::optional<SmallVector<InterestingMemoryOperand, 1>>>
+          MemInfo = TTI.getTgtMemIntrinsic(II);
+      if (MemInfo.first != std::nullopt && MemInfo.first->PtrVal) {


Suggested change

if (MemInfo.first != std::nullopt && MemInfo.first->PtrVal) {

if (MemInfo.first.PtrVal) {

Check PtrVal here too.

HankChang736 · 2025-08-20T14:19:04Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

+                  std::optional<SmallVector<InterestingMemoryOperand, 1>>>
+            MemInfo;
+        MemInfo = TTI->getTgtMemIntrinsic(II);
+        if (MemInfo.second != std::nullopt)


Suggested change

if (MemInfo.second != std::nullopt)

if (!MemInfo.second.empty())

Check the SmallVector is empty or not.

This commit drops std::optional in getTgtMemIntrinsic return type, use nullptr in PtrVal to identify empty MemIntrinsicInfo and empty() to identify empty SmallVector<InterestingMemoryOperand, 1>.

HankChang736 · 2025-08-21T08:29:29Z

@topperc
I just update the return type in getTgtMemIntrinsic() with dropping std::optional, now it use nullptr in PtrVal to check empty MemIntrinsicInfo and use empty() to check empty SmallVector.

The commit that include this update is f2abced960e7be54d8eb3aa6557dbc472c4f40f2

topperc · 2025-08-25T21:44:18Z

llvm/include/llvm/Analysis/InterestingMemoryOperand.h

@@ -0,0 +1,55 @@
+//===--------- Definition of the InterestingMemoryOperand class -*- C++


Don't wrap this line

topperc · 2025-08-25T21:46:40Z

llvm/include/llvm/Analysis/TargetTransformInfo.h

  /// if false is returned.
-  LLVM_ABI bool getTgtMemIntrinsic(IntrinsicInst *Inst,
-                                   MemIntrinsicInfo &Info) const;
+  LLVM_ABI std::pair<MemIntrinsicInfo, SmallVector<InterestingMemoryOperand, 1>>


Could we make InterestingMemoryOperand a member of MemIntrinsicInfo and keep the original signature?

I’m worried the original signature by returning bool might be ambiguous for targets that never produce SmallVector<InterestingMemoryOperand, 1>.
With SmallVector<InterestingMemoryOperand, 1> embedded in MemIntrinsicInfo, an empty vector can mean either:

The target didn’t implement getTgtMemIntrinsic.

The target did implement it and there are truly no SmallVector<InterestingMemoryOperand, 1>.

Aren't those both possibilities if you return a SmallVector as part of a pair? Is the difference between those two cases meaningful for the current use case? Should we go back to the std::optional?

Yes, it is.

But returning a pair essentially makes the Transform passes that use getTgtMemIntrinsic directly check whether the returned SmallVector is empty or not with an if condition. On the other hand, embedding SmallVector inside MemIntrinsicInfo while keeping the original signature bool getTgtMemIntrinsic(IntrinsicInst *Inst, MemIntrinsicInfo &Info) introduces another subtlety: since returning true only guarantees that MemIntrinsicInfo is filled, but some targets like AArch64, PowerPC and AMDGPU don't fill SmallVector.
This means that passes like AddressSanitizer, which only rely on SmallVector, could end up accessing it even when it is empty, leading to ambiguity or potential misuse. As a result, we would still need to double-check whether the SmallVector is empty to ensure the target actually provided the corresponding data.

I think making InterestingMemoryOperand a member of MemIntrinsicInfo is also an approach. Since it keeps the original implementation, but it requires a double-check like this in AddressSanitizer pass :

if (TTI.getTgtMemIntrinsic(Inst, Info) && !Info.Interesting.empty())

Also it needs to check the PtrVal in EarlyCSE to ensure whether the target really fills the information for it since those targets that support getTgtMemIntrinsic all fills PtrVal.

if (TTI.getTgtMemIntrinsic(Inst, Info) && Info.PtrVal)

Doesn't EarlyCSE check ParseMemoryInst::isValid() to check if the PtrVal is null before doing anything with it.

This means that passes like AddressSanitizer, which only rely on SmallVector, could end up accessing it even when it is empty, leading to ambiguity or potential misuse. As a result, we would still need to double-check whether the SmallVector is empty to ensure the target actually provided the corresponding data.

Doesn't AddressSanitizer need to check the vector is non-empty before using it regardless of where it is stored?

Ah, I see — that makes sense now.

For EarlyCSE, ParseMemoryInst::isValid() already ensures that PtrVal is non-null before using it, so we don’t actually need an extra check there.
And for AddressSanitizer, it explicitly verifies that the vector is non-empty before using it. So it looks like we can indeed make InterestingMemoryOperand a member of MemIntrinsicInfo safely.

Thanks for pointing this out — it clears up my earlier concern.

topperc · 2025-08-25T21:47:49Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

 bool AddressSanitizer::instrumentFunction(Function &F,
-                                          const TargetLibraryInfo *TLI) {
+                                          const TargetLibraryInfo *TLI,
+					  const TargetTransformInfo *TTI) {


weird formatting

topperc · 2025-08-25T21:50:09Z

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

-        if (IntrInfo.PtrVal == OperandVal)
+      std::pair<MemIntrinsicInfo, SmallVector<InterestingMemoryOperand, 1>>
+          MemInfo = TTI.getTgtMemIntrinsic(II);
+      if (MemInfo.first.PtrVal != nullptr) {


Is OperandVal always non-null? If so, it could never match a null MemInfo.first.PtrVal.

If we do this check, then use && to join the two ifs instead of nesting them.

I'm not sure if OperandVal is always non-null here. Maybe we keep this check since we make SmallVector a member of MemIntrinsicInfo and keep the original signatures.

…nsicInfo This commit make SmallVector<InterestingMemoryOperand> a member of MemIntrinsicInfo so that we can keep the original interface of getTgtMemIntrinsic.

HankChang736 · 2025-08-28T14:12:48Z

@topperc
I make SmallVector<InterestingMemoryOperand, 1> a member of MemIntrinsic Info, and fix the passes that use getTgtMemIntrinsic in commit e89d6ba.

topperc · 2025-09-09T19:22:29Z

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp


 bool GCNTTIImpl::getTgtMemIntrinsic(IntrinsicInst *Inst,
-                                       MemIntrinsicInfo &Info) const {
+                                    MemIntrinsicInfo &Info) const {


Please revert these formatting changes

topperc · 2025-09-09T19:22:38Z

llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp

    break;
  }
-
  return false;


Please revert this change

topperc · 2025-09-09T19:22:45Z

llvm/lib/Target/PowerPC/PPCTargetTransformInfo.h

  bool canSaveCmp(Loop *L, BranchInst **BI, ScalarEvolution *SE, LoopInfo *LI,
                  DominatorTree *DT, AssumptionCache *AC,
                  TargetLibraryInfo *LibInfo) const override;
+


revert this change

topperc · 2025-09-09T19:23:35Z

llvm/lib/Transforms/Scalar/EarlyCSE.cpp

      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst)) {
        IntrID = II->getIntrinsicID();
-        if (TTI.getTgtMemIntrinsic(II, Info))
+        if (TTI.getTgtMemIntrinsic(II, Info)) {


Drop curly braces

topperc · 2025-09-09T19:23:46Z

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

      if (TTI.getTgtMemIntrinsic(II, IntrInfo) && IntrInfo.PtrVal) {
-        AccessTy.AddrSpace
-          = IntrInfo.PtrVal->getType()->getPointerAddressSpace();
+        AccessTy.AddrSpace =


revert this formatting change

HankChang736 · 2025-09-10T06:05:03Z

@topperc
The format is changed in commit 9f26e22, 665ebae and 766d8c5.

topperc

LGTM

HankChang736 · 2025-09-10T14:06:38Z

I just created a pr in llvm upstream.

llvm#157863

…), C)) (llvm#155141) Hi, I compared the following LLVM IR with GCC and Clang, and there is a small difference between the two. The LLVM IR is: ``` define i64 @test_smin_neg_one(i64 %a) { %1 = tail call i64 @llvm.smin.i64(i64 %a, i64 -1) %retval.0 = xor i64 %1, -1 ret i64 %retval.0 } ``` GCC generates: ``` cmp x0, 0 csinv x0, xzr, x0, ge ret ``` Clang generates: ``` cmn x0, #1 csinv x8, x0, xzr, lt mvn x0, x8 ret ``` Clang keeps flipping x0 through x8 unnecessarily. So I added the following folds to DAGCombiner: fold (xor (smax(x, C), C)) -> select (x > C), xor(x, C), 0 fold (xor (smin(x, C), C)) -> select (x < C), xor(x, C), 0 alive2: https://alive2.llvm.org/ce/z/gffoir --------- Co-authored-by: Yui5427 <[email protected]> Co-authored-by: Matt Arsenault <[email protected]> Co-authored-by: Simon Pilgrim <[email protected]>

llvm#158769) …52471)" This reverts commit e4eccd6. This was causing ASan failures in some situations involving unordered multimap containers. Details and a reproducer were posted on the original PR (llvm#152471).

A few improvements to logging when lldb-dap is started in **Server Mode** AND when the **`lldb-dap.logFolder`** setting is used (not `lldb-dap.log-path`). ### Improvement #1 **Avoid the prompt of restarting the server when starting each debug session.** That prompt is caused by the combination of the following facts: 1. The log filename changes every time a new debug session is starting (see [here](https://github.com/llvm/llvm-project/blob/9d6062c490548a5e6fea103e010ab3c9bc73a86d/lldb/tools/lldb-dap/src-ts/logging.ts#L47)) 2. The log filename is passed to the server via an environment variable called "LLDBDAP_LOG" (see [here](https://github.com/llvm/llvm-project/blob/9d6062c490548a5e6fea103e010ab3c9bc73a86d/lldb/tools/lldb-dap/src-ts/debug-adapter-factory.ts#L263-L269)) 3. All environment variables are put into the "spawn info" variable (see [here](https://github.com/llvm/llvm-project/blob/9d6062c490548a5e6fea103e010ab3c9bc73a86d/lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts#L170-L172)). 4. The old and new "spawn info" are compared to decide if a prompt should show (see [here](https://github.com/llvm/llvm-project/blob/9d6062c490548a5e6fea103e010ab3c9bc73a86d/lldb/tools/lldb-dap/src-ts/lldb-dap-server.ts#L107-L110)). The fix is to remove the "LLDBDAP_LOG" from the "spawn info" variable, so that the same server can be reused if the log path is the only thing that has changed. ### Improvement llvm#2 **Avoid log file conflict when multiple users share a machine and start server in the same second.** The problem: If two users start lldb-dap server in the same second, they will share the same log path. The first user will create the log file. The second user will find that they cannot access the same file, so their server will fail to start. The fix is to add a part of the VS Code session ID to the log filename. ### Improvement llvm#3 **Avoid restarting the server when the order of environment variables changed.** This is done by sorting the environment variables before putting them into the "spawn info".

Specifically, `X & M ?= C --> (C << clz(M)) ?= (X << clz(M))` where M is a non-empty sequence of ones starting at the least significant bit with the remainder zero and C is a constant subset of M that cannot be materialised into a SUBS (immediate). Proof: https://alive2.llvm.org/ce/z/haqdJ4. This improves the comparison in isinf, for example: ```cpp int isinf(float x) { return __builtin_isinf(x); } ``` Before: ``` isinf: fmov w9, s0 mov w8, #2139095040 and w9, w9, #0x7fffffff cmp w9, w8 cset w0, eq ret ``` After: ``` isinf: fmov w9, s0 mov w8, #-16777216 cmp w8, w9, lsl #1 cset w0, eq ret ```

kito-cheng reviewed Aug 18, 2025

View reviewed changes

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp Outdated Show resolved Hide resolved

llvm/lib/Transforms/Scalar/EarlyCSE.cpp Outdated Show resolved Hide resolved

llvm/lib/Transforms/Scalar/EarlyCSE.cpp Show resolved Hide resolved

HankChang736 added 2 commits August 18, 2025 15:05

Address Kito's comment

d88808b

Address Kito's 2nd comment

867b0d7

topperc reviewed Aug 19, 2025

View reviewed changes

Move InterestingMemoryOperand from Transforms header to Analysis

7e574c7

HankChang736 commented Aug 20, 2025

View reviewed changes

Drop std::optional in getTgtMemIntrinsic return type

f2abced

This commit drops std::optional in getTgtMemIntrinsic return type, use nullptr in PtrVal to identify empty MemIntrinsicInfo and empty() to identify empty SmallVector<InterestingMemoryOperand, 1>.

topperc reviewed Aug 25, 2025

View reviewed changes

[TTI] Make SmallVector<InterestingMemoryOperand> a member of MemIntri…

e89d6ba

…nsicInfo This commit make SmallVector<InterestingMemoryOperand> a member of MemIntrinsicInfo so that we can keep the original interface of getTgtMemIntrinsic.

topperc reviewed Sep 9, 2025

View reviewed changes

HankChang736 added 2 commits September 10, 2025 13:50

Fix format with Craig's comment

9f26e22

Fix format

665ebae

Fix more format

766d8c5

topperc approved these changes Sep 10, 2025

View reviewed changes

HankChang736 closed this Sep 10, 2025

	std::optional<SmallVector<InterestingMemoryOperand, 1>>>
	std::pair<MemIntrinsicInfo,
	SmallVector<InterestingMemoryOperand, 1>>

	if (MemInfo.first != std::nullopt) {
	if (MemInfo.first.PtrVal) {

	if (MemInfo.first != std::nullopt && MemInfo.first->PtrVal) {
	if (MemInfo.first.PtrVal) {

	if (MemInfo.second != std::nullopt)
	if (!MemInfo.second.empty())

		@@ -0,0 +1,55 @@
		//===--------- Definition of the InterestingMemoryOperand class -*- C++

Change getTgtMemIntrinsic interface with returning a pair #1

Change getTgtMemIntrinsic interface with returning a pair #1

Uh oh!

Conversation

HankChang736 commented Aug 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HankChang736 commented Aug 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topperc Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HankChang736 Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HankChang736 commented Aug 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HankChang736 commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topperc left a comment

Choose a reason for hiding this comment

Uh oh!

HankChang736 commented Sep 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

topperc Aug 26, 2025 •

edited

Loading

HankChang736 Aug 27, 2025 •

edited

Loading

HankChang736 commented Sep 10, 2025 •

edited

Loading