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

[RISCV] Remove incomplete PRE_DEC/POST_DEC code for XTHeadMemIdx. #76922

Merged
merged 3 commits into from
Jan 4, 2024

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Jan 4, 2024

As far as I can tell if getIndexedAddressParts received an ISD::SUB, the constant would be negated. So IsInc should be set to true since the SUB was effectively converted to ADD. This means we should never use PRE_DEC/POST_DEC.

No tests are affected because DAGCombine aggressively turns SUB with constant into ADD so no lit test has a SUB reach getIndexedAddressParts.

As far as I can tell if getIndexedAddressParts received an ISD::SUB,
the constant would be negated. So IsInc should be set to true since
the SUB was effectively converted to ADD.  This means we should never
use PRE_DEC/POST_DEC.

No tests are affected because DAGCombine aggressively turns SUB with
constant into ADD so no lit test has a SUB reach getIndexedAddressParts.
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 4, 2024

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

Author: Craig Topper (topperc)

Changes

As far as I can tell if getIndexedAddressParts received an ISD::SUB, the constant would be negated. So IsInc should be set to true since the SUB was effectively converted to ADD. This means we should never use PRE_DEC/POST_DEC.

No tests are affected because DAGCombine aggressively turns SUB with constant into ADD so no lit test has a SUB reach getIndexedAddressParts.


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

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp (+4-6)
  • (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+3-3)
diff --git a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
index bfa3bf3cc74e2b..befa9e1159bef1 100644
--- a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
@@ -763,14 +763,12 @@ bool RISCVDAGToDAGISel::tryIndexedLoad(SDNode *Node) {
     return false;
 
   EVT LoadVT = Ld->getMemoryVT();
-  bool IsPre = (AM == ISD::PRE_INC || AM == ISD::PRE_DEC);
-  bool IsPost = (AM == ISD::POST_INC || AM == ISD::POST_DEC);
+  assert(AM == ISD::PRE_INC || AM == ISD::POST_INC &&
+         "Unexpected addressing mode");
+  bool IsPre = AM == ISD::PRE_INC;
+  bool IsPost = AM == ISD::POST_INC;
   int64_t Offset = C->getSExtValue();
 
-  // Convert decrements to increments by a negative quantity.
-  if (AM == ISD::PRE_DEC || AM == ISD::POST_DEC)
-    Offset = -Offset;
-
   // The constants that can be encoded in the THeadMemIdx instructions
   // are of the form (sign_extend(imm5) << imm2).
   int64_t Shift;
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index c8a94adcd91c6a..0a886fe70eeaed 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -1350,8 +1350,7 @@ RISCVTargetLowering::RISCVTargetLowering(const TargetMachine &TM,
   }
 
   if (Subtarget.hasVendorXTHeadMemIdx()) {
-    for (unsigned im = (unsigned)ISD::PRE_INC; im != (unsigned)ISD::POST_DEC;
-         ++im) {
+    for (unsigned im : {ISD::PRE_INC, ISD::POST_INC}) {
       setIndexedLoadAction(im, MVT::i8, Legal);
       setIndexedStoreAction(im, MVT::i8, Legal);
       setIndexedLoadAction(im, MVT::i16, Legal);
@@ -19288,7 +19287,8 @@ bool RISCVTargetLowering::getIndexedAddressParts(SDNode *Op, SDValue &Base,
     if (!isLegalIndexedOffset)
       return false;
 
-    IsInc = (Op->getOpcode() == ISD::ADD);
+    // Constant for SUB was negated earlier.
+    IsInc = true;
     Offset = Op->getOperand(1);
     return true;
   }

@topperc topperc requested a review from ptomsich January 4, 2024 08:52
Copy link

github-actions bot commented Jan 4, 2024

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

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

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

I don't see any error and it seems that AArch64 is the same.
@zixuan-wu Please help double check this, thanks!

@@ -19288,7 +19287,8 @@ bool RISCVTargetLowering::getIndexedAddressParts(SDNode *Op, SDValue &Base,
if (!isLegalIndexedOffset)
return false;

IsInc = (Op->getOpcode() == ISD::ADD);
// Constant for SUB was negated earlier.
IsInc = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

If IsInc is always true, then we don't need this parameter in getIndexedAddressParts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor

@ptomsich ptomsich left a comment

Choose a reason for hiding this comment

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

Reviewed and LGTM.
I didn't rerun tests, as that was already done by Craig.

Copy link
Contributor

@asb asb left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

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

LGTM.

@topperc topperc merged commit a960703 into llvm:main Jan 4, 2024
3 of 4 checks passed
@topperc topperc deleted the pr/xtheadmemidx branch January 4, 2024 17:51
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

5 participants