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] Fix fewerElementsVectorPhi to insert after G_PHIs #87927

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

dfszabo
Copy link
Contributor

@dfszabo dfszabo commented Apr 7, 2024

Currently the inserted mergelike instructions will be inserted at the location of the G_PHI. Seems like the behaviour was correct before, but the rework done in https://reviews.llvm.org/D114198 forgot to include the part which makes sure the instructions will be inserted after all the G_PHIs.

@llvmbot
Copy link
Member

llvmbot commented Apr 7, 2024

@llvm/pr-subscribers-llvm-globalisel

Author: Dávid Ferenc Szabó (dfszabo)

Changes

Currently the inserted mergelike instructions will be inserted at the location of the G_PHI. Seems like the behaviour was correct before, but the rework done in https://reviews.llvm.org/D114198 forgot to include the part which makes sure the instructions will be inserted after all the G_PHIs.


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp (+4)
diff --git a/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
index 95c6a359e52ec6..9412fa85cd3856 100644
--- a/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
@@ -4265,6 +4265,10 @@ LegalizerHelper::fewerElementsVectorPhi(GenericMachineInstr &MI,
     }
   }
 
+  // Set the insert point after the existing PHIs
+  MachineBasicBlock &MBB = *MI.getParent();
+  MIRBuilder.setInsertPt(MBB, MBB.getFirstNonPHI());
+
   // Merge small outputs into MI's def.
   if (NumLeftovers) {
     mergeMixedSubvectors(MI.getReg(0), OutputRegs);

@dfszabo
Copy link
Contributor Author

dfszabo commented Apr 7, 2024

This issue came up with our downstream backend, but the problematic examples are passing with AArch64. Not easy to create a test for this, but on the other hand the change is fairly self evident.

@AreaZR
Copy link
Contributor

AreaZR commented Apr 7, 2024

This issue came up with our downstream backend, but the problematic examples are passing with AArch64. Not easy to create a test for this, but on the other hand the change is fairly self evident.

There is a failing unit test. Maybe that can give an idea of how this changes things.

@dfszabo
Copy link
Contributor Author

dfszabo commented Apr 7, 2024

This issue came up with our downstream backend, but the problematic examples are passing with AArch64. Not easy to create a test for this, but on the other hand the change is fairly self evident.

There is a failing unit test. Maybe that can give an idea of how this changes things.

Yes indeed, the test fix show now nicely the expected behavior.

Currently the inserted mergelike instructions will be inserted at the location of the G_PHI. Seems like the behaviour was correct before, but the rework done in https://reviews.llvm.org/D114198 forgot to include the part which makes sure the instructions will be inserted after all the G_PHIs.
@AreaZR
Copy link
Contributor

AreaZR commented Apr 14, 2024

Ping?

@arsenm arsenm merged commit 2347020 into llvm:main Apr 15, 2024
4 checks passed
bazuzi pushed a commit to bazuzi/llvm-project that referenced this pull request Apr 15, 2024
…87927)

Currently the inserted mergelike instructions will be inserted at the
location of the G_PHI. Seems like the behaviour was correct before, but
the rework done in https://reviews.llvm.org/D114198 forgot to include
the part which makes sure the instructions will be inserted after all
the G_PHIs.
@AreaZR
Copy link
Contributor

AreaZR commented Apr 15, 2024

/cherry-pick 2347020

@llvmbot
Copy link
Member

llvmbot commented Apr 15, 2024

/cherry-pick 2347020

Error: Command failed due to missing milestone.

aniplcc pushed a commit to aniplcc/llvm-project that referenced this pull request Apr 15, 2024
…87927)

Currently the inserted mergelike instructions will be inserted at the
location of the G_PHI. Seems like the behaviour was correct before, but
the rework done in https://reviews.llvm.org/D114198 forgot to include
the part which makes sure the instructions will be inserted after all
the G_PHIs.
AreaZR pushed a commit to AreaZR/llvm-project that referenced this pull request Apr 18, 2024
…87927)

Currently the inserted mergelike instructions will be inserted at the
location of the G_PHI. Seems like the behaviour was correct before, but
the rework done in https://reviews.llvm.org/D114198 forgot to include
the part which makes sure the instructions will be inserted after all
the G_PHIs.

(cherry picked from commit 2347020)
AreaZR pushed a commit to AreaZR/llvm-project that referenced this pull request Apr 23, 2024
…87927)

Currently the inserted mergelike instructions will be inserted at the
location of the G_PHI. Seems like the behaviour was correct before, but
the rework done in https://reviews.llvm.org/D114198 forgot to include
the part which makes sure the instructions will be inserted after all
the G_PHIs.

(cherry picked from commit 2347020)
tstellar pushed a commit to AreaZR/llvm-project that referenced this pull request Apr 24, 2024
…87927)

Currently the inserted mergelike instructions will be inserted at the
location of the G_PHI. Seems like the behaviour was correct before, but
the rework done in https://reviews.llvm.org/D114198 forgot to include
the part which makes sure the instructions will be inserted after all
the G_PHIs.

(cherry picked from commit 2347020)
@pointhex pointhex mentioned this pull request May 7, 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.

4 participants