[PATCH] D27573: [X86] Fix bug in r288804.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6414-6415
               Alignment);
-          SDValue Brdcst = DAG.getNode(X86ISD::VBROADCAST, dl,
+          Brdcst = DAG.getNode(X86ISD::VBROADCAST, dl,
                                        MVT::getVectorVT(CVT, Repeat), Ld);
         } else if (SplatBitSize == 32 || SplatBitSize == 64) {
----------------
X86ISD::VBROADCAST requires a memory operand only if the target is not AVX2.
On AVX2, we should still be able to select a register-register variant of VBROADCAST from a X86ISD::VBROADCAST.
Basically (at least on AVX2) I don't think that a X86ISD::VBROADCAST is problematic even if we have load with multiple uses.



================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6435-6436
               Alignment);
-          SDValue Brdcst = DAG.getNode(X86ISD::VBROADCAST, dl,
+          Brdcst = DAG.getNode(X86ISD::VBROADCAST, dl,
                                        MVT::getVectorVT(CVT, Repeat), Ld);
         } else if (SplatBitSize > 64) {
----------------
Would it be possible to check in advance if the Ld node is already used?
In case, we would know in advance that the check at line 6455 would fail. That would let us early exit before we even create a node for the broadcast.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6445-6449
           Ld = DAG.getLoad(
               MVT::getVectorVT(CVT, NumElm), dl, DAG.getEntryNode(), VCP,
               MachinePointerInfo::getConstantPool(DAG.getMachineFunction()),
               Alignment);
+          Brdcst = DAG.getNode(X86ISD::SUBV_BROADCAST, dl, VT, Ld);
----------------
Same as above. Early exit if we know in advance that Ld has already uses.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6450-6455
+        } else {
+          return SDValue();
         }
+        // If the load already had a use, we can't fold it into the broadcast.
+        // So fallback to the original state.
+        if (!Ld.hasOneUse())
----------------
I think it is safe to remove the `else` part.
You can always check if `Brdcst` is null at line 6455.

I think you can write something like this:
```
if (Brdcst && !Ld.hasOneUse())
```


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6453-6456
+        // If the load already had a use, we can't fold it into the broadcast.
+        // So fallback to the original state.
+        if (!Ld.hasOneUse())
+          return SDValue();
----------------
Phabricator doesn't let me delete this comment...


https://reviews.llvm.org/D27573