[Clang][HLSL][GVN] Prevent phi codgen of isTokenLike types

[farzonl]

fixes #161754

When the GVN pass is PerformLoadPRE we need to check if it might be doing this load on a token like type. This is because if we don't GVN will use the SSAUpdater to insert a phi node to reduce duplicate resource.getpointer calls.

Because in an earlier commit: 01c0a84 we made the verifier error with PHI nodes cannot have token type!

This test case will fail today if we try to perform this load optimization
https://godbolt.org/z/xM69fY8zM

This will impact clang aswell because isTokenLikeTy also checks for isTokenTy Clang is likely also failing validation with token types but just doesn't have a test case because the validator would error if it were in a phi node.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Farzon Lotfi (farzonl)

Changes

fixes #161754

When the GVN pass is PerformLoadPRE we need to check if it might be doing this load on a token like type. This is because if we don't GVN will use the SSAUpdater to insert a phi node to reduce duplicate resource.getpointer calls.

Because in an earlier commit: 01c0a84 we made the verifier error with PHI nodes cannot have token type!

This test case will fail today if we try to perform this load optimization
https://godbolt.org/z/xM69fY8zM

This will impact clang aswell because isTokenLikeTy also checks for isTokenTy Clang is likely also failing validation with token types but just doesn't have a test case because the validator would error if it were in a phi node.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/GVN.cpp (+3)
• (added) llvm/test/Transforms/GVN/PRE/no-pre-load-for-token-like.ll (+34)

diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index b9b5b5823d780..0fa99eb375a47 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -1659,6 +1659,9 @@ bool GVNPass::PerformLoadPRE(LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
   // that we only have to insert *one* load (which means we're basically moving
   // the load, not inserting a new one).
 
+  if (Load->getType()->isTokenLikeTy())
+    return false;
+
   SmallPtrSet<BasicBlock *, 4> Blockers(llvm::from_range, UnavailableBlocks);
 
   // Let's find the first basic block with more than one predecessor.  Walk
diff --git a/llvm/test/Transforms/GVN/PRE/no-pre-load-for-token-like.ll b/llvm/test/Transforms/GVN/PRE/no-pre-load-for-token-like.ll
new file mode 100644
index 0000000000000..7fc56e8b5d8f1
--- /dev/null
+++ b/llvm/test/Transforms/GVN/PRE/no-pre-load-for-token-like.ll
@@ -0,0 +1,34 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
+; RUN: opt -S -passes=gvn %s | FileCheck %s
+
+; NOTE: A test to confirm GVN doesn't introduce phis for pre loads of token
+; NOTE: like types. This implies the CHECKS should exactly match the IR.
+
+define ptr @main() {
+; CHECK-LABEL: define ptr @main() {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    br i1 false, label %[[ENTRY_IF_END_I_CRIT_EDGE:.*]], label %[[IF_THEN_I:.*]]
+; CHECK:       [[ENTRY_IF_END_I_CRIT_EDGE]]:
+; CHECK-NEXT:    br label %[[IF_END_I:.*]]
+; CHECK:       [[IF_THEN_I]]:
+; CHECK-NEXT:    [[TMP0:%.*]] = load target("dx.RawBuffer", half, 1, 0), ptr null, align 4
+; CHECK-NEXT:    [[TMP1:%.*]] = tail call ptr @llvm.dx.resource.getpointer.p0.tdx.RawBuffer_f16_1_0t(target("dx.RawBuffer", half, 1, 0) [[TMP0]], i32 0)
+; CHECK-NEXT:    br label %[[IF_END_I]]
+; CHECK:       [[IF_END_I]]:
+; CHECK-NEXT:    [[TMP2:%.*]] = load target("dx.RawBuffer", half, 1, 0), ptr null, align 4
+; CHECK-NEXT:    [[TMP3:%.*]] = tail call ptr @llvm.dx.resource.getpointer.p0.tdx.RawBuffer_f16_1_0t(target("dx.RawBuffer", half, 1, 0) [[TMP2]], i32 0)
+; CHECK-NEXT:    ret ptr [[TMP3]]
+;
+entry:
+  br i1 false, label %if.end.i, label %if.then.i
+
+if.then.i:                                        ; preds = %entry
+  %0 = load target("dx.RawBuffer", half, 1, 0), ptr null, align 4
+  %1 = tail call ptr @llvm.dx.resource.getpointer.p0.tdx.RawBuffer_f16_1_0t(target("dx.RawBuffer", half, 1, 0) %0, i32 0)
+  br label %if.end.i
+
+if.end.i:                                         ; preds = %if.then.i, %entry
+  %2 = load target("dx.RawBuffer", half, 1, 0), ptr null, align 4
+  %3 = tail call ptr @llvm.dx.resource.getpointer.p0.tdx.RawBuffer_f16_1_0t(target("dx.RawBuffer", half, 1, 0) %2, i32 0)
+  ret ptr %3
+}

[Icohedron]

The change looks good to me and it successfully compiles the reduced shader I sent you over Teams. However, the original DML shaders are still having the same issue - hitting the same assertion. So this PR does not completely fix the issue.

I will send you some more shaders to test and debug.

[farzonl]

The change looks good to me and it successfully compiles the reduced shader I sent you over Teams. However, the original DML shaders are still having the same issue - hitting the same assertion. So this PR does not completely fix the issue.

I will send you some more shaders to test and debug.

I have a fix to move the check up two functions, but I'm still trying to work out a minified reproductions. llvm-reduce is getting stuck in an infinite loop.

diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index 0fa99eb375a4..995fd1458598 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -2003,6 +2006,9 @@ bool GVNPass::processNonLocalLoad(LoadInst *Load) {
       Load->getParent()->getParent()->hasFnAttribute(
           Attribute::SanitizeHWAddress))
     return false;
+  
+  if (Load->getType()->isTokenLikeTy())
+    return false;
 
   // Step 1: Find the non-local dependencies of the load.
   LoadDepVect Deps;