InferAddressSpaces: Handle llvm.is.constant #102010

arsenm · 2024-08-05T16:28:22Z

No description provided.

arsenm · 2024-08-05T16:28:40Z

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

Join @arsenm and the rest of your teammates on Graphite

llvmbot · 2024-08-05T16:28:56Z

@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-llvm-transforms

Author: Matt Arsenault (arsenm)

Changes

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

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp (+3-1)
(added) llvm/test/Transforms/InferAddressSpaces/AMDGPU/is.constant.ll (+35)

diff --git a/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp b/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
index 2ddf24be67702..67d4e7b5ee146 100644
--- a/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
+++ b/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
@@ -401,7 +401,8 @@ bool InferAddressSpacesImpl::rewriteIntrinsicOperands(IntrinsicInst *II,
     II->setCalledFunction(NewDecl);
     return true;
   }
-  case Intrinsic::prefetch: {
+  case Intrinsic::prefetch:
+  case Intrinsic::is_constant: {
     Function *NewDecl =
         Intrinsic::getDeclaration(M, II->getIntrinsicID(), {NewV->getType()});
     II->setArgOperand(0, NewV);
@@ -426,6 +427,7 @@ void InferAddressSpacesImpl::collectRewritableIntrinsicOperands(
   switch (IID) {
   case Intrinsic::ptrmask:
   case Intrinsic::objectsize:
+  case Intrinsic::is_constant:
     appendsFlatAddressExpressionToPostorderStack(II->getArgOperand(0),
                                                  PostorderStack, Visited);
     break;
diff --git a/llvm/test/Transforms/InferAddressSpaces/AMDGPU/is.constant.ll b/llvm/test/Transforms/InferAddressSpaces/AMDGPU/is.constant.ll
new file mode 100644
index 0000000000000..767681a281792
--- /dev/null
+++ b/llvm/test/Transforms/InferAddressSpaces/AMDGPU/is.constant.ll
@@ -0,0 +1,35 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -passes=infer-address-spaces %s | FileCheck %s
+
+define i1 @is_constant_global_to_flat(ptr addrspace(1) %ptr) {
+; CHECK-LABEL: define i1 @is_constant_global_to_flat(
+; CHECK-SAME: ptr addrspace(1) [[PTR:%.*]]) {
+; CHECK-NEXT:    [[RET:%.*]] = call i1 @llvm.is.constant.p1(ptr addrspace(1) [[PTR]])
+; CHECK-NEXT:    ret i1 [[RET]]
+;
+  %cast = addrspacecast ptr addrspace(1) %ptr to ptr
+  %ret = call i1 @llvm.is.constant.p0(ptr %cast)
+  ret i1 %ret
+}
+
+define i1 @is_constant_local_to_flat(ptr addrspace(3) %ptr) {
+; CHECK-LABEL: define i1 @is_constant_local_to_flat(
+; CHECK-SAME: ptr addrspace(3) [[PTR:%.*]]) {
+; CHECK-NEXT:    [[RET:%.*]] = call i1 @llvm.is.constant.p3(ptr addrspace(3) [[PTR]])
+; CHECK-NEXT:    ret i1 [[RET]]
+;
+  %cast = addrspacecast ptr addrspace(3) %ptr to ptr
+  %ret = call i1 @llvm.is.constant.p0(ptr %cast)
+  ret i1 %ret
+}
+
+define i1 @is_constant_private_to_flat(ptr addrspace(5) %ptr) {
+; CHECK-LABEL: define i1 @is_constant_private_to_flat(
+; CHECK-SAME: ptr addrspace(5) [[PTR:%.*]]) {
+; CHECK-NEXT:    [[RET:%.*]] = call i1 @llvm.is.constant.p5(ptr addrspace(5) [[PTR]])
+; CHECK-NEXT:    ret i1 [[RET]]
+;
+  %cast = addrspacecast ptr addrspace(5) %ptr to ptr
+  %ret = call i1 @llvm.is.constant.p0(ptr %cast)
+  ret i1 %ret
+}

rampitec

Add some tests where argument is not a pointer?

tschuett · 2024-08-05T17:00:59Z

llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp

+    if (Ptr->getType()->isPtrOrPtrVectorTy()) {
+      appendsFlatAddressExpressionToPostorderStack(Ptr, PostorderStack,
+                                                   Visited);
+    }


unnecessary braces?

Multiple lines

multiple lines is not listed as an excuse in the coding style

"However, braces should be used in cases where the omission of braces harm the readability and maintainability of the code."

and in my judgment this covers any situation with multiple lines

How could it hurt readability?

Because unlike a compiler I read by lines, and not statements.

The length of that section of the style guide is absurd. It should never be wrong to include braces. That's a lot simpler than parsing out whatever that is

It should never be wrong to include braces.

Having to deal with google style that demands braces everywhere, and LLVM which does not want them, my personal choice is "whatever the style guide says". It may not always be a perfect choice, but it's not worth anyone's time to argue over specific instances, where the right choice is ambiguous or is a matter of personal preference. I wish we could delegate braces/no-braces decisions to clang-format, too, but I don't think it currently handles that.

I'd stick with the style guide defaults and either have the braces removed, or a comment added to the body. Perhaps, making the function name shorter, and avoiding line-wrapping would address your readability concerns about braces/no-braces here, too.

This rule is so open ended it's useless. If clang-format doesn't do this, it's not a rule. I'm not wasting more CI cycles to touch this

rampitec

LGTM modulo braces comment.

Artem-B · 2024-08-05T18:42:07Z

llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp

+    if (Ptr->getType()->isPtrOrPtrVectorTy()) {
+      appendsFlatAddressExpressionToPostorderStack(Ptr, PostorderStack,
+                                                   Visited);
+    }


It should never be wrong to include braces.

Having to deal with google style that demands braces everywhere, and LLVM which does not want them, my personal choice is "whatever the style guide says". It may not always be a perfect choice, but it's not worth anyone's time to argue over specific instances, where the right choice is ambiguous or is a matter of personal preference. I wish we could delegate braces/no-braces decisions to clang-format, too, but I don't think it currently handles that.

I'd stick with the style guide defaults and either have the braces removed, or a comment added to the body. Perhaps, making the function name shorter, and avoiding line-wrapping would address your readability concerns about braces/no-braces here, too.

tschuett · 2024-08-05T19:06:03Z

From the clang-format documentation: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm

RemoveBracesLLVM (Boolean)

Artem-B · 2024-08-05T19:35:57Z

From the clang-format documentation: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm

Thank you for the pointer. Unfortunately, it looks like there's a good reason it does not seem to be on by default:

Setting this option to true could lead to incorrect code formatting due to clang-format’s lack of complete semantic information.

arsenm · 2024-08-05T20:10:35Z

Merge activity

Aug 5, 4:10 PM EDT: @arsenm started a stack merge that includes this pull request via Graphite.
Aug 5, 4:17 PM EDT: Graphite rebased this pull request as part of a merge.
Aug 5, 4:20 PM EDT: @arsenm merged this pull request with Graphite.

tschuett · 2024-08-06T02:52:14Z

Braces still there and no response to @Artem-B? I don't like that.

arsenm · 2024-08-06T06:39:50Z

Braces still there and no response to @Artem-B? I don't like that.

I posted the reply yesterday but graphite apparently didn't submit it

arsenm requested review from Artem-B, d0k, jlebar, krzysz00, Pierre-vh, rampitec, shiltian and yxsamliu August 5, 2024 16:28

arsenm added backend:AMDGPU llvm:transforms labels Aug 5, 2024 — with Graphite App

arsenm mentioned this pull request Aug 5, 2024

InferAddressSpaces: Handle prefetch intrinsic #101982

Merged

arsenm mentioned this pull request Aug 5, 2024

InferAddressSpaces: Handle masked load and store intrinsics #102007

Merged

arsenm marked this pull request as ready for review August 5, 2024 16:28

rampitec reviewed Aug 5, 2024

View reviewed changes

arsenm force-pushed the users/arsenm/infer-address-spaces-handle-is-constant branch from 2dde2ad to 227f60e Compare August 5, 2024 16:54

tschuett reviewed Aug 5, 2024

View reviewed changes

rampitec approved these changes Aug 5, 2024

View reviewed changes

Artem-B approved these changes Aug 5, 2024

View reviewed changes

arsenm force-pushed the users/arsenm/infer-address-spaces-masked-load-store branch from 0cd0fd7 to fd7b3c2 Compare August 5, 2024 20:14

Base automatically changed from users/arsenm/infer-address-spaces-masked-load-store to main August 5, 2024 20:17

InferAddressSpaces: Handle llvm.is.constant

71a166f

arsenm force-pushed the users/arsenm/infer-address-spaces-handle-is-constant branch from 227f60e to 71a166f Compare August 5, 2024 20:17

arsenm merged commit 2ef553c into main Aug 5, 2024
3 of 5 checks passed

arsenm deleted the users/arsenm/infer-address-spaces-handle-is-constant branch August 5, 2024 20:20

InferAddressSpaces: Handle llvm.is.constant #102010

InferAddressSpaces: Handle llvm.is.constant #102010

Uh oh!

Conversation

arsenm commented Aug 5, 2024

Uh oh!

arsenm commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rampitec left a comment

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!

rampitec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tschuett commented Aug 5, 2024

Uh oh!

Artem-B commented Aug 5, 2024

Uh oh!

arsenm commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

tschuett commented Aug 6, 2024

Uh oh!

arsenm commented Aug 6, 2024

Uh oh!

Uh oh!

arsenm commented Aug 5, 2024 •

edited

Loading

llvmbot commented Aug 5, 2024 •

edited

Loading

arsenm commented Aug 5, 2024 •

edited

Loading