[PATCH] D47878: [DAGCombiner] Fix for PR37667

[Nirav Dave via Phabricator via llvm-commits]

niravd added a comment.

This also makes sense to me. Just to build on Sanjays comment, I think recasting DataValues as boolean is nicer.

NodeToMask = Op.getNode();
bool HasValue = false;
for (unsigned i = 0; i < NodeToMask->getNumValues(); ++i) {

  MVT VT = SDValue(NodeToMask, i).getSimpleValueType();
  if (VT != MVT::Glue && VT != MVT::Other){
    if(HasValue) {
      NodeToMask = nullptr;
      return false;
    }
    HasValue = true;

}

In https://reviews.llvm.org/D47878#1125018, @spatel wrote:

> I'm not familiar with this transform, but the proposal makes sense. But I think it would be easier to follow the logic if it was written as:
>
>   // Also ensure that the node to be masked only produces one data result.
>   NodeToMask = Op.getNode();
>   unsigned DataValues = 0;
>   for (unsigned i = 0; i < NodeToMask->getNumValues(); ++i) {
>     MVT VT = SDValue(NodeToMask, i).getSimpleValueType();
>     if (VT != MVT::Glue && VT != MVT::Other)
>       ++DataValues;
>     if (DataValues > 1) {
>       NodeToMask = nullptr;
>       return false;
>     }
>   }
>   assert(DataValues == 1 && "Node to be masked has no data result?");
>   
>   
>
> Also, I added tests for both bugs (just in case there was logic difference there) with https://reviews.llvm.org/rL334199. Please use utils/update_llc_test_checks.py to update the assertions and rebase here.




In https://reviews.llvm.org/D47878#1125018, @spatel wrote:

> I'm not familiar with this transform, but the proposal makes sense. But I think it would be easier to follow the logic if it was written as:
>
>   // Also ensure that the node to be masked only produces one data result.
>   NodeToMask = Op.getNode();
>   unsigned DataValues = 0;
>   for (unsigned i = 0; i < NodeToMask->getNumValues(); ++i) {
>     MVT VT = SDValue(NodeToMask, i).getSimpleValueType();
>     if (VT != MVT::Glue && VT != MVT::Other)
>       ++DataValues;
>     if (DataValues > 1) {
>       NodeToMask = nullptr;
>       return false;
>     }
>   }
>   assert(DataValues == 1 && "Node to be masked has no data result?");
>   
>   
>
> Also, I added tests for both bugs (just in case there was logic difference there) with https://reviews.llvm.org/rL334199. Please use utils/update_llc_test_checks.py to update the assertions and rebase here.





https://reviews.llvm.org/D47878