[PATCH] D47878: [DAGCombiner] Fix for PR37667
Nirav Dave via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 7 08:22:48 PDT 2018
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
More information about the llvm-commits
mailing list