[X86][NFC]Use ref instead copy in for loop for SDValue::op_values()

[XinWang10]

Summary

The for range loop in function LowerFMINIMUM_FMAXIMUM, it enumerate Op->op_values() with copy instead of ref, every element here is a SDValue which contains 1 pointer SDNode and an unsigned value, it's better to use ref here as what the author did in SelectionDAGNodes.h.

[craig.topper]

Pretty sure the op is an SDValue not an SDUse.

[XinWang10]

This comment was removed by XinWang10.

[XinWang10]

Oh, I see. Thanks for pointing.

[craig.topper]

We pass SDValue by value all over the SelectionD
AG code. And it looks pretty easy for a compiler to see the unsigned field is unused. Does this change really make a difference?

[goldstein.w.n]

It should be const ref I think. But 16 byte by value is usually reasonably efficient

[XinWang10]

In D151977#4390071, @craig.topper wrote:

We pass SDValue by value all over the SelectionD
AG code. And it looks pretty easy for a compiler to see the unsigned field is unused. Does this change really make a difference?

I don't know about if compiler will do this opt to ignore the unused field for a class obj. Previously I mistook the enumerated element with SDUse which has more fields, but now the SDValue only has two fields, I also think this will make big difference. And if compiler would do the opt you said, maybe the program will go worse.

[XinWang10]

In D151977#4390081, @goldstein.w.n wrote:

It should be const ref I think. But 16 byte by value is usually reasonably efficient

Thanks for pointing! I'm interested in where the info '16 byte' from, actually I searched for some while but didn't get useful info. I used to think that 8 byte could be reasonable because it can be fit in internal type.

[craig.topper]

In D151977#4390131, @XinWang10 wrote:

In D151977#4390071, @craig.topper wrote:

We pass SDValue by value all over the SelectionD
AG code. And it looks pretty easy for a compiler to see the unsigned field is unused. Does this change really make a difference?

I don't know about if compiler will do this opt to ignore the unused field for a class obj.

I would expect SROA to break up the memcpy into scalar stores after that dead store elimination should be able to remove it.

For the pointer, I expect store to load forwarding should make it we don’t have to load from the stored copy. Then the store also becomes eligible for dead store elimination.

[XinWang10]

In D151977#4390175, @craig.topper wrote:

In D151977#4390131, @XinWang10 wrote:

In D151977#4390071, @craig.topper wrote:

We pass SDValue by value all over the SelectionD
AG code. And it looks pretty easy for a compiler to see the unsigned field is unused. Does this change really make a difference?

I don't know about if compiler will do this opt to ignore the unused field for a class obj.

I would expect SROA to break up the memcpy into scalar stores after that dead store elimination should be able to remove it.

For the pointer, I expect store to load forwarding should make it we don’t have to load from the stored copy. Then the store also becomes eligible for dead store elimination.

Thanks for the explanation! I will give up this revision.

llvm/lib/Target/X86/X86ISelLowering.cpp

     return Use->getOpcode() != ISD::FMA && Use->getOpcode() != ISD::STRICT_FMA;
   };
   if (llvm::any_of(V->uses(), IsNotFMA))
     return SDValue();
 
   SmallVector<SDValue, 8> Ops;
   EVT VT = V.getValueType();
   EVT EltVT = VT.getVectorElementType();
-  for (auto Op : V->op_values()) {
+  for (auto &Op : V->op_values()) {
     if (auto *Cst = dyn_cast<ConstantFPSDNode>(Op)) {
       Ops.push_back(DAG.getConstantFP(-Cst->getValueAPF(), SDLoc(Op), EltVT));
     } else {
       assert(Op.isUndef());
       Ops.push_back(DAG.getUNDEF(EltVT));
     }
   }
 
   SDNode *NV = DAG.getNodeIfExists(ISD::BUILD_VECTOR, DAG.getVTList(VT), Ops);
   if (!NV)
     return SDValue();
 
   // If an inverted version cannot be eliminated, choose it instead of the
   // original version.
   if (llvm::any_of(NV->uses(), IsNotFMA))
     return SDValue(NV, 0);
 
   // If the inverted version also can be eliminated, we have to consistently
   // prefer one of the values. We prefer a constant with a negative value on
   // the first place.
   // N.B. We need to skip undefs that may precede a value.
-  for (auto op : V->op_values()) {
+  for (auto &op : V->op_values()) {
     if (auto *Cst = dyn_cast<ConstantFPSDNode>(op)) {
       if (Cst->isNegative())
         return SDValue();
       break;
     }
   }
   return SDValue(NV, 0);
 }