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

[GISel] Add LookThroughInstrs for getIConstantVRegVal and getIConstan… #68327

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

michaelmaitland
Copy link
Contributor

…tVRegSExtVal

The implementation of these methods uses getIConstantVRegValWithLookThrough with LookThroughInstrs argument set to false. By adding the optional argument to getIConstantVRegVal and getIConstantVRegSExtVal we can take advantage of the already built look through functionality.

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 5, 2023

@llvm/pr-subscribers-llvm-globalisel

Changes

…tVRegSExtVal

The implementation of these methods uses getIConstantVRegValWithLookThrough with LookThroughInstrs argument set to false. By adding the optional argument to getIConstantVRegVal and getIConstantVRegSExtVal we can take advantage of the already built look through functionality.


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/GlobalISel/Utils.cpp (+5-3)
diff --git a/llvm/lib/CodeGen/GlobalISel/Utils.cpp b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
index 473c3f452f8b1d9..bc97acd9d76f07c 100644
--- a/llvm/lib/CodeGen/GlobalISel/Utils.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
@@ -290,9 +290,10 @@ void llvm::reportGISelFailure(MachineFunction &MF, const TargetPassConfig &TPC,
 }
 
 std::optional<APInt> llvm::getIConstantVRegVal(Register VReg,
-                                               const MachineRegisterInfo &MRI) {
+                                               const MachineRegisterInfo &MRI,
+                                               bool LookThroughInstrs = false) {
   std::optional<ValueAndVReg> ValAndVReg = getIConstantVRegValWithLookThrough(
-      VReg, MRI, /*LookThroughInstrs*/ false);
+      VReg, MRI, LookThroughInstrs);
   assert((!ValAndVReg || ValAndVReg->VReg == VReg) &&
          "Value found while looking through instrs");
   if (!ValAndVReg)
@@ -301,7 +302,8 @@ std::optional<APInt> llvm::getIConstantVRegVal(Register VReg,
 }
 
 std::optional<int64_t>
-llvm::getIConstantVRegSExtVal(Register VReg, const MachineRegisterInfo &MRI) {
+llvm::getIConstantVRegSExtVal(Register VReg, const MachineRegisterInfo &MRI,
+                              bool LookThroughInstrs = false) {
   std::optional<APInt> Val = getIConstantVRegVal(VReg, MRI);
   if (Val && Val->getBitWidth() <= 64)
     return Val->getSExtValue();

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

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

…tVRegSExtVal

The implementation of these methods uses getIConstantVRegValWithLookThrough
with LookThroughInstrs argument set to false. By adding the optional
argument to getIConstantVRegVal and getIConstantVRegSExtVal we can take
advantage of the already built look through functionality.
@tschuett
Copy link
Member

tschuett commented Oct 5, 2023

LGTM.

@michaelmaitland michaelmaitland merged commit 28ae42e into llvm:main Oct 20, 2023
3 checks passed
Guzhu-AMD pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Oct 26, 2023
Local branch amd-gfx b6fe60a Merged main:1d4601a1ef84 into amd-gfx:13ef5174ada6
Remote branch main 28ae42e [GISel] Add LookThroughInstrs for getIConstantVRegVal and getIConstan… (llvm#68327)
topperc added a commit that referenced this pull request Nov 4, 2023
…IConstan… (#68327)"

This reverts commit 28ae42e.

The assert in getIConstantVRegVal was not updated for this change.
The ValAndVReg->VReg == VReg check fails if any look through happens.

RISC-V was the only target using the lookthrough functionality, but I'm
not sure it was needed so I'm removing that too.
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.

3 participants