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][NFCI] Use isADDLike helper for or_is_add PatFrag #81137

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

asb
Copy link
Contributor

@asb asb commented Feb 8, 2024

This should be equivalent.

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 8, 2024

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

Author: Alex Bradbury (asb)

Changes

This should be equivalent.


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

1 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.td (+1-5)
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.td b/llvm/lib/Target/RISCV/RISCVInstrInfo.td
index 518982441e7c0..aa84a4553ec1f 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.td
@@ -1249,11 +1249,7 @@ def : PatGprUimmLog2XLen<sra, SRAI>;
 // Select 'or' as ADDI if the immediate bits are known to be 0 in $rs1. This
 // can improve compressibility.
 def or_is_add : PatFrag<(ops node:$lhs, node:$rhs), (or node:$lhs, node:$rhs),[{
-  if (N->getFlags().hasDisjoint())
-    return true;
-  KnownBits Known0 = CurDAG->computeKnownBits(N->getOperand(0), 0);
-  KnownBits Known1 = CurDAG->computeKnownBits(N->getOperand(1), 0);
-  return KnownBits::haveNoCommonBitsSet(Known0, Known1);
+  return CurDAG->isADDLike(SDValue(N, 0));
 }]>;
 def : PatGprSimm12<or_is_add, ADDI>;
 

Copy link
Collaborator

@preames preames 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
Copy link
Collaborator

topperc commented Feb 8, 2024

Is it equivalent? Aren't there some special cases handled in isAddLIke? See haveNoCommonBitsSetCommutative in SelectionDAG.cpp.

@asb
Copy link
Contributor Author

asb commented Feb 8, 2024

Is it equivalent? Aren't there some special cases handled in isAddLIke? See haveNoCommonBitsSetCommutative in SelectionDAG.cpp.

Good point, I'd assumed that SelectionDAG::haveNoCommonBitsSet was a trivial wrapper over KnownBits;:HaveNoCommonBitsSet. So there may be some functional change. Are you pointing out that the NFCI tag is likely incorrect and we probably want tests for any new patterns picked up, or do you have correctness concerns? I haven't stepped through the haveNoCommonBitsSetCommutative logic, but isADDLike would be buggy (per its documented semantics) if the or couldn't freely be replaced with an add when it returns true.

@topperc
Copy link
Collaborator

topperc commented Feb 8, 2024

Is it equivalent? Aren't there some special cases handled in isAddLIke? See haveNoCommonBitsSetCommutative in SelectionDAG.cpp.

Good point, I'd assumed that SelectionDAG::haveNoCommonBitsSet was a trivial wrapper over KnownBits;:HaveNoCommonBitsSet. So there may be some functional change. Are you pointing out that the NFCI tag is likely incorrect and we probably want tests for any new patterns picked up, or do you have correctness concerns? I haven't stepped through the haveNoCommonBitsSetCommutative logic, but isADDLike would be buggy (per its documented semantics) if the or couldn't freely be replaced with an add when it returns true.

I don't have correctness concerns, just that we should have new tests.

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