Local: Handle noalias_addrspace in combineMetadata

[arsenm]

This should act like range.

Previously ConstantRangeList assumed a 64-bit range. Now query from the actual entries. This also means that the empty range has no bitwidth, so move asserts to avoid checking the bitwidth of empty ranges.

[arsenm]

• Local: Handle noalias.addrspace in copyMetadataForLoad #103939
• Local: Handle noalias_addrspace in combineMetadata #103938 👈 (View in Graphite)
• [RFC] IR: Define noalias.addrspace metadata #102461 : 2 other dependent PRs (#102462 , #102599 )
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: Matt Arsenault (arsenm)

Changes

This should act like range.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Utils/Local.cpp (+6-1)
• (modified) llvm/test/Transforms/SimplifyCFG/hoist-with-metadata.ll (+4-2)

diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index efb02fdec56d7..bbc8933d387e1 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -3319,6 +3319,10 @@ void llvm::combineMetadata(Instruction *K, const Instruction *J,
         if (DoesKMove)
           K->setMetadata(Kind, MDNode::getMergedProfMetadata(KMD, JMD, K, J));
         break;
+      case LLVMContext::MD_noalias_addrspace:
+        if (DoesKMove)
+          K->setMetadata(Kind, MDNode::getMostGenericRange(JMD, KMD));
+        break;
     }
   }
   // Set !invariant.group from J if J has it. If both instructions have it
@@ -3360,7 +3364,8 @@ void llvm::combineMetadataForCSE(Instruction *K, const Instruction *J,
                          LLVMContext::MD_prof,
                          LLVMContext::MD_nontemporal,
                          LLVMContext::MD_noundef,
-                         LLVMContext::MD_mmra};
+                         LLVMContext::MD_mmra,
+                         LLVMContext::MD_noalias_addrspace};
   combineMetadata(K, J, KnownIDs, KDominatesJ);
 }
 
diff --git a/llvm/test/Transforms/SimplifyCFG/hoist-with-metadata.ll b/llvm/test/Transforms/SimplifyCFG/hoist-with-metadata.ll
index 18aa5c9e044a9..f8985e78c0ca5 100644
--- a/llvm/test/Transforms/SimplifyCFG/hoist-with-metadata.ll
+++ b/llvm/test/Transforms/SimplifyCFG/hoist-with-metadata.ll
@@ -319,7 +319,7 @@ out:
 define void @hoist_noalias_addrspace_both(i1 %c, ptr %p, i64 %val) {
 ; CHECK-LABEL: @hoist_noalias_addrspace_both(
 ; CHECK-NEXT:  if:
-; CHECK-NEXT:    [[T:%.*]] = atomicrmw add ptr [[P:%.*]], i64 [[VAL:%.*]] seq_cst, align 8
+; CHECK-NEXT:    [[T:%.*]] = atomicrmw add ptr [[P:%.*]], i64 [[VAL:%.*]] seq_cst, align 8, !noalias.addrspace [[META7:![0-9]+]]
 ; CHECK-NEXT:    ret void
 ;
 if:
@@ -361,7 +361,7 @@ out:
 define void @hoist_noalias_addrspace_switch(i64 %i, ptr %p, i64 %val) {
 ; CHECK-LABEL: @hoist_noalias_addrspace_switch(
 ; CHECK-NEXT:  out:
-; CHECK-NEXT:    [[T:%.*]] = atomicrmw add ptr [[P:%.*]], i64 [[VAL:%.*]] seq_cst, align 8
+; CHECK-NEXT:    [[T:%.*]] = atomicrmw add ptr [[P:%.*]], i64 [[VAL:%.*]] seq_cst, align 8, !noalias.addrspace [[META8:![0-9]+]]
 ; CHECK-NEXT:    ret void
 ;
   switch i64 %i, label %bb0 [
@@ -398,4 +398,6 @@ out:
 ; CHECK: [[RNG4]] = !{i32 0, i32 10}
 ; CHECK: [[META5]] = !{i64 4}
 ; CHECK: [[META6]] = !{float 2.500000e+00}
+; CHECK: [[META7]] = !{i32 5, i32 6}
+; CHECK: [[META8]] = !{i32 4, i32 8}
 ;.

[nikic]

It looks like this still includes the LangRef/IR changes.

[frasercrmck]

Should we also have a test for how it combines non-contiguous ranges?

[frasercrmck]

I notice we're moving a bunch of asserts around. I think that needs explaining in the commit message. However, it feels like something's wrong.

Does insert perhaps need to be updated so that it inserts 32-bit values depending on the range type? Would that fix us having mixed 64- and 32-bit range values? Is that even possible right now - what if we want to insert into an empty range? We can't know what bitwidth to use.

[arsenm]

An empty range has no bitwidth. I don't think it's worth adding an explicit field for bit width to give a bit width to the empty range

[frasercrmck]

I agree that an empty range has no bitwidth. I missed that ConstantRangeList only deals with the insertion of pre-existing ConstantRanges. I think it's okay on balance to require users to only insert/union/intersect ConstantRanges of matching bitwidths.

I think a good way of expressing this would be to keep the asserts where they are but have getBitWidth return a value that represents "none" or "empty" which always compares == true with bitwidths of other non-empty ranges. That's probably not worth the effort, though. What you have now is probably okay.

[frasercrmck]

If the range list is empty, should this continue to return 64? Otherwise at best it'll assert and at worst it'll return garbage. An optional could also represent this scenario.

If there's a new precondition that you can only call getBitWidth on a non-empty range, that should be documented.

[frasercrmck]

I agree that an empty range has no bitwidth. I missed that ConstantRangeList only deals with the insertion of pre-existing ConstantRanges. I think it's okay on balance to require users to only insert/union/intersect ConstantRanges of matching bitwidths.

I think a good way of expressing this would be to keep the asserts where they are but have getBitWidth return a value that represents "none" or "empty" which always compares == true with bitwidths of other non-empty ranges. That's probably not worth the effort, though. What you have now is probably okay.

[arsenm]

ping

[nikic]

LGTM