[GISel] Fix #77762: extend correct source registers in combiner helper rule extend_through_phis

[widlarizer]

Since we already know which register we want to extend, we don't have to ask its defining MI about it

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-llvm-globalisel

Author: Emil J (ekliptik)

Changes

Since we already know which register we want to extend, we don't have to ask its defining MI about it

---

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

1 Files Affected:

• (modified) llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp (+3-2)

diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index fc2793bd7a133d..866774bda3fb14 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -3939,7 +3939,8 @@ void CombinerHelper::applyExtendThroughPhis(MachineInstr &MI,
   SmallSetVector<MachineInstr *, 8> SrcMIs;
   SmallDenseMap<MachineInstr *, MachineInstr *, 8> OldToNewSrcMap;
   for (unsigned SrcIdx = 1; SrcIdx < MI.getNumOperands(); SrcIdx += 2) {
-    auto *SrcMI = MRI.getVRegDef(MI.getOperand(SrcIdx).getReg());
+    auto SrcReg = MI.getOperand(SrcIdx).getReg();
+    auto *SrcMI = MRI.getVRegDef(SrcReg);
     if (!SrcMIs.insert(SrcMI))
       continue;
 
@@ -3952,7 +3953,7 @@ void CombinerHelper::applyExtendThroughPhis(MachineInstr &MI,
     Builder.setInsertPt(*SrcMI->getParent(), InsertPt);
     Builder.setDebugLoc(MI.getDebugLoc());
     auto NewExt = Builder.buildExtOrTrunc(ExtMI->getOpcode(), ExtTy,
-                                          SrcMI->getOperand(0).getReg());
+                                          SrcReg);
     OldToNewSrcMap[SrcMI] = NewExt;
   }
 

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff b7770befee37feca3d732d6daf9513c62f75c5f0 b2c3b074b3ba04cd88cffc9eb38c170121d32242 -- llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index 866774bda3..34849d17b7 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -3952,8 +3952,7 @@ void CombinerHelper::applyExtendThroughPhis(MachineInstr &MI,
 
     Builder.setInsertPt(*SrcMI->getParent(), InsertPt);
     Builder.setDebugLoc(MI.getDebugLoc());
-    auto NewExt = Builder.buildExtOrTrunc(ExtMI->getOpcode(), ExtTy,
-                                          SrcReg);
+    auto NewExt = Builder.buildExtOrTrunc(ExtMI->getOpcode(), ExtTy, SrcReg);
     OldToNewSrcMap[SrcMI] = NewExt;
   }
 

[aemerson]

Thanks for the fix. This needs a test, and it's not immediately clear to me why this change is correct vs the old code?

[widlarizer]

Looks like ths PR and issue are unlinked. This PR fixes #77762. I will describe the problem in clearer terms in the issue

• Add test

[aemerson]

LGTM, can you fix the clang format issue before you merge?

[aemerson]

LGTM, can you fix the clang format issue before you merge?

It's fine actually, I'm going to refactor some of this to use the new GPhi wrapper recently added.

[widlarizer]

Oops, I was manually renaming the variables since I wrote and verified the test when rebased on our fork which had an older version of the check statements. I pushed it with the expectation that CI tests will run before merge and catch it if there's an issue but I guess they don't auto run for new contributors