Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPIR-V] Ensure that we don't have a dangling BlockAddress constants after internal intrinsic 'spv_switch' is processed #92390

Merged

Conversation

VyacheslavLevytskyy
Copy link
Contributor

@VyacheslavLevytskyy VyacheslavLevytskyy commented May 16, 2024

After internal intrinsic 'spv_switch' is processed we need to delete G_BLOCK_ADDR instructions that were generated to keep track of the corresponding basic blocks. If we just delete G_BLOCK_ADDR instructions with BlockAddress operands, this leaves their BasicBlock counterparts in a "address taken" status. This would make AsmPrinter to generate a series of unneeded labels of a "Address of block that was removed by CodeGen" kind. This PR is to ensure that we don't have a dangling BlockAddress constants by zapping the BlockAddress nodes, and only after that proceed with erasing G_BLOCK_ADDR instructions.

See also #87823 for more details.

@llvmbot
Copy link

llvmbot commented May 16, 2024

@llvm/pr-subscribers-backend-spir-v

Author: Vyacheslav Levytskyy (VyacheslavLevytskyy)

Changes

After internal intrinsic 'spv_switch' is processed we need to delete G_BLOCK_ADDR instructions that were generated to keep track of the corresponding basic blocks. If we just delete G_BLOCK_ADDR instructions with BlockAddress operands, this leaves their BasicBlock counterparts in a "address taken" status. This would make AsmPrinter to generate a series of unneeded labels of a "Address of block that was removed by CodeGen" kind. This PR is to ensure that we don't have a dangling BlockAddress constants by zapping the BlockAddress nodes, and only after that proceed with erasing G_BLOCK_ADDR instructions.


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

2 Files Affected:

  • (modified) llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp (+18-1)
  • (modified) llvm/test/CodeGen/SPIRV/branching/switch-range-check.ll (+3)
diff --git a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
index 6ee5d90d9afe1..9bff23dd96668 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
@@ -602,8 +602,25 @@ static void processSwitches(MachineFunction &MF, SPIRVGlobalRegistry *GR,
         ToEraseMI.insert(Next);
     }
   }
-  for (MachineInstr *BlockAddrI : ToEraseMI)
+
+  // If we just delete G_BLOCK_ADDR instructions with BlockAddress operands,
+  // this leaves their BasicBlock counterparts in a "address taken" status. This
+  // would make AsmPrinter to generate a series of unneeded labels of a "Address
+  // of block that was removed by CodeGen" kind. Let's first ensure that we
+  // don't have a dangling BlockAddress constants by zapping the BlockAddress
+  // nodes, and only after that proceed with erasing G_BLOCK_ADDR instructions.
+  Constant *Replacement =
+      ConstantInt::get(Type::getInt32Ty(MF.getFunction().getContext()), 1);
+  for (MachineInstr *BlockAddrI : ToEraseMI) {
+    if (BlockAddrI->getOpcode() == TargetOpcode::G_BLOCK_ADDR) {
+      BlockAddress *BA = const_cast<BlockAddress *>(
+          BlockAddrI->getOperand(1).getBlockAddress());
+      BA->replaceAllUsesWith(
+          ConstantExpr::getIntToPtr(Replacement, BA->getType()));
+      BA->destroyConstant();
+    }
     BlockAddrI->eraseFromParent();
+  }
 }
 
 static bool isImplicitFallthrough(MachineBasicBlock &MBB) {
diff --git a/llvm/test/CodeGen/SPIRV/branching/switch-range-check.ll b/llvm/test/CodeGen/SPIRV/branching/switch-range-check.ll
index 85a4d4db089cb..f8ce15323aacf 100644
--- a/llvm/test/CodeGen/SPIRV/branching/switch-range-check.ll
+++ b/llvm/test/CodeGen/SPIRV/branching/switch-range-check.ll
@@ -1,10 +1,13 @@
 ; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
 ; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %}
 
+; CHECK: OpFunction
 ; CHECK: %[[#Var:]] = OpPhi
 ; CHECK: OpSwitch %[[#Var]] %[[#]] [[#]] %[[#]] [[#]] %[[#]] [[#]] %[[#]] [[#]] %[[#]] [[#]] %[[#]] [[#]] %[[#]] [[#]] %[[#]] [[#]] %[[#]] [[#]] %[[#]] [[#]] %[[#]] [[#]] %[[#]] [[#]] %[[#]]
 ; CHECK-COUNT-11: OpBranch
 ; CHECK-NOT: OpBranch
+; CHECK: OpReturn
+; CHECK-NEXT: OpFunctionEnd
 
 define spir_func void @foo(i64 noundef %addr, i64 noundef %as) {
 entry:

Copy link
Contributor

@Keenuts Keenuts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing! 😊

@VyacheslavLevytskyy VyacheslavLevytskyy merged commit e3e0613 into llvm:main May 17, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants