Skip to content

Conversation

@topperc
Copy link
Collaborator

@topperc topperc commented Jan 8, 2025

I think SDNodeIterator primarily exists because GraphTraits requires an iterator that dereferences to SDNode*. op_iterator dereferences to SDUse* which is implicitly convertible to SDValue.

This piece of code can use SDValue instead of SDNode* so we should prefer to use the the more common op_iterator.

I think SDNodeIterator primarily exists because GraphTraits requires
an iterator that dereferences to SDNode*. op_iterator dereferences to
SDUse* which is implicitly convertible to SDValue.

This piece of code can use SDValue instead of SDNode* so we should
prefer to use the the more common op_iterator.
@topperc topperc requested review from RKSimon and arsenm January 8, 2025 18:11
@llvmbot llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Jan 8, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 8, 2025

@llvm/pr-subscribers-llvm-selectiondag

Author: Craig Topper (topperc)

Changes

I think SDNodeIterator primarily exists because GraphTraits requires an iterator that dereferences to SDNode*. op_iterator dereferences to SDUse* which is implicitly convertible to SDValue.

This piece of code can use SDValue instead of SDNode* so we should prefer to use the the more common op_iterator.


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp (+1-2)
diff --git a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
index 9f57884eae04df..5099833ce86cbf 100644
--- a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
@@ -2986,8 +2986,7 @@ bool TargetLowering::SimplifyDemandedBits(
       DemandedBits.isSubsetOf(Known.Zero | Known.One)) {
     // Avoid folding to a constant if any OpaqueConstant is involved.
     const SDNode *N = Op.getNode();
-    for (SDNode *Op :
-         llvm::make_range(SDNodeIterator::begin(N), SDNodeIterator::end(N))) {
+    for (SDValue Op : N->ops()) {
       if (auto *C = dyn_cast<ConstantSDNode>(Op))
         if (C->isOpaque())
           return false;

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

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

LGTM with one optional minor

const SDNode *N = Op.getNode();
for (SDNode *Op :
llvm::make_range(SDNodeIterator::begin(N), SDNodeIterator::end(N))) {
for (SDValue Op : N->ops()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Op->ops();

@topperc topperc merged commit e2449f1 into llvm:main Jan 9, 2025
8 checks passed
@topperc topperc deleted the pr/op_iterator_simplifydemanded branch January 9, 2025 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llvm:SelectionDAG SelectionDAGISel as well

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants