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

[GlobalISel] Make IRTranslator able to handle PHIs with empty types. #73235

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

dfszabo
Copy link
Contributor

@dfszabo dfszabo commented Nov 23, 2023

SelectionDAG already handle this since e53b7d1.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 23, 2023

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-llvm-globalisel

Author: Dávid Ferenc Szabó (dfszabo)

Changes

SelectionDAG already handle this since e53b7d1.


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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp (+2)
  • (modified) llvm/test/CodeGen/Generic/zero-sized-array.ll (+1)
diff --git a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
index 62450e4c43ff3e6..ac6fa144fa46bcf 100644
--- a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -3118,6 +3118,8 @@ void IRTranslator::finishPendingPhis() {
 #endif // ifndef NDEBUG
   for (auto &Phi : PendingPHIs) {
     const PHINode *PI = Phi.first;
+      if (PI->getType()->isEmptyTy())
+        continue;
     ArrayRef<MachineInstr *> ComponentPHIs = Phi.second;
     MachineBasicBlock *PhiMBB = ComponentPHIs[0]->getParent();
     EntryBuilder->setDebugLoc(PI->getDebugLoc());
diff --git a/llvm/test/CodeGen/Generic/zero-sized-array.ll b/llvm/test/CodeGen/Generic/zero-sized-array.ll
index 05fa5686106df92..75d65173a08091b 100644
--- a/llvm/test/CodeGen/Generic/zero-sized-array.ll
+++ b/llvm/test/CodeGen/Generic/zero-sized-array.ll
@@ -1,4 +1,5 @@
 ; RUN: llc < %s
+; RUN: llc -O0 -mtriple=aarch64 -global-isel < %s
 ; PR9900
 
 ; NVPTX does not support zero sized type arg

Copy link

github-actions bot commented Nov 23, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@dfszabo dfszabo force-pushed the main-global-isel-empty-type-with-phi branch 2 times, most recently from 134e2a4 to ad72e10 Compare November 23, 2023 14:12
@@ -1,4 +1,5 @@
; RUN: llc < %s
; RUN: llc -O0 -mtriple=aarch64 -global-isel < %s
; PR9900

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no phi in this test? Should also probably put the test in a real target's test/CodeGen/*/GlobalISel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is at line 50. My reason to put here is that this change is not target specific. On other hand the default target is x86 which crash with this, hence I changed it to aarch64 for this test for now, should be removed once x86 would not crash on this. Therefore I am not sure if we should move this to specific target test folder, but I understand this might interfere with the generic nature of these tests.

x86 will crash on this test even without the change with the same error message which is

RUN: at line 1: /home/david/work/llvm-community/build/bin/llc < /home/david/work/llvm-community/llvm-project/llvm/test/CodeGen/Generic/zero-sized-array.ll
+ /home/david/work/llvm-community/build/bin/llc
RUN: at line 2: /home/david/work/llvm-community/build/bin/llc -O0 -global-isel < /home/david/work/llvm-community/llvm-project/llvm/test/CodeGen/Generic/zero-sized-array.ll
+ /home/david/work/llvm-community/build/bin/llc -O0 -global-isel
llc: /home/david/work/llvm-community/llvm-project/llvm/include/llvm/CodeGen/MachineRegisterInfo.h:665: const llvm::TargetRegisterClass *llvm::MachineRegisterInfo::getRegClass(llvm::Register) const: Assertion `isa<const TargetRegisterClass *>(VRegInfo[Reg.id()].first) && "Register class not set, wrong accessor"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /home/david/work/llvm-community/build/bin/llc -O0 -global-isel
1.	Running pass 'Function Pass Manager' on module '<stdin>'.
2.	Running pass 'Fast Tile Register Preconfigure' on function '@f'
 #0 0x00007f7ed261c1ad llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/david/work/llvm-community/llvm-project/llvm/lib/Support/Unix/Signals.inc:723:11
 #1 0x00007f7ed261c6eb PrintStackTraceSignalHandler(void*) /home/david/work/llvm-community/llvm-project/llvm/lib/Support/Unix/Signals.inc:798:1
 #2 0x00007f7ed261a60b llvm::sys::RunSignalHandlers() /home/david/work/llvm-community/llvm-project/llvm/lib/Support/Signals.cpp:104:5
 #3 0x00007f7ed261cf11 SignalHandler(int) /home/david/work/llvm-community/llvm-project/llvm/lib/Support/Unix/Signals.inc:413:1
 #4 0x00007f7ed7d313c0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x143c0)
 #5 0x00007f7ed1cef03b raise /build/glibc-sMfBJT/glibc-2.31/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
 #6 0x00007f7ed1cce859 abort /build/glibc-sMfBJT/glibc-2.31/stdlib/abort.c:81:7
 #7 0x00007f7ed1cce729 get_sysdep_segment_value /build/glibc-sMfBJT/glibc-2.31/intl/loadmsgcat.c:509:8
 #8 0x00007f7ed1cce729 _nl_load_domain /build/glibc-sMfBJT/glibc-2.31/intl/loadmsgcat.c:970:34
 #9 0x00007f7ed1ce0006 (/lib/x86_64-linux-gnu/libc.so.6+0x34006)
#10 0x00007f7eda0f3c3d llvm::MachineRegisterInfo::getRegClass(llvm::Register) const /home/david/work/llvm-community/llvm-project/llvm/include/llvm/CodeGen/MachineRegisterInfo.h:0:5
#11 0x00007f7eda150dc6 (anonymous namespace)::X86FastPreTileConfig::runOnMachineFunction(llvm::MachineFunction&) /home/david/work/llvm-community/llvm-project/llvm/lib/Target/X86/X86FastPreTileConfig.cpp:670:36
#12 0x00007f7ed6621631 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /home/david/work/llvm-community/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:93:8
#13 0x00007f7ed310b2d4 llvm::FPPassManager::runOnFunction(llvm::Function&) /home/david/work/llvm-community/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1443:23
#14 0x00007f7ed3110655 llvm::FPPassManager::runOnModule(llvm::Module&) /home/david/work/llvm-community/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1489:16
#15 0x00007f7ed310bca8 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /home/david/work/llvm-community/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1558:23
#16 0x00007f7ed310b7d3 llvm::legacy::PassManagerImpl::run(llvm::Module&) /home/david/work/llvm-community/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:541:16
#17 0x00007f7ed3110961 llvm::legacy::PassManager::run(llvm::Module&) /home/david/work/llvm-community/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1685:3
#18 0x0000000000230328 compileModule(char**, llvm::LLVMContext&) /home/david/work/llvm-community/llvm-project/llvm/tools/llc/llc.cpp:761:41
#19 0x000000000022e4fa main /home/david/work/llvm-community/llvm-project/llvm/tools/llc/llc.cpp:434:13
#20 0x00007f7ed1cd00b3 __libc_start_main /build/glibc-sMfBJT/glibc-2.31/csu/../csu/libc-start.c:342:3
#21 0x000000000022dc5e _start (/home/david/work/llvm-community/build/bin/llc+0x22dc5e)
/home/david/work/llvm-community/build/test/CodeGen/Generic/Output/zero-sized-array.ll.script: line 2: 307428 Aborted                 (core dumped) /home/david/work/llvm-community/build/bin/llc -O0 -global-isel < /home/david/work/llvm-community/llvm-project/llvm/test/CodeGen/Generic/zero-sized-array.ll

--

So this crash is independent of the code change.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is not really such a thing as a generic codegen test. You have to have a target, and this demonstrates yet another example of why. This will fail if aarch64 isn't built. You should add a copy in test/AArch64/GlobalISel and not add a target run line here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Moved and minimized it into test/AArch64/GlobalISel folder.

@dfszabo dfszabo force-pushed the main-global-isel-empty-type-with-phi branch from ad72e10 to 9fb3120 Compare December 1, 2023 17:32
@dfszabo dfszabo requested a review from arsenm January 8, 2024 09:37
@dfszabo dfszabo force-pushed the main-global-isel-empty-type-with-phi branch from 9fb3120 to 5756d08 Compare January 9, 2024 13:12
@@ -0,0 +1,16 @@
; RUN: llc < %s -O0 -global-isel -mtriple=aarch64
Copy link
Contributor

Choose a reason for hiding this comment

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

Should include checks (probably MIR checks, with -stop-after=irtranslator)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Included it.

@dfszabo dfszabo force-pushed the main-global-isel-empty-type-with-phi branch from 5756d08 to 9ffc669 Compare January 12, 2024 08:23
Copy link
Collaborator

@qcolombet qcolombet left a comment

Choose a reason for hiding this comment

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

Nice catch!

@arsenm arsenm merged commit 0ff3d72 into llvm:main Jan 15, 2024
5 checks passed
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
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.

None yet

4 participants