[PATCH] Remove spurious mask operations from AArch64 add->compares on 16 and 8 bit values

[Quentin Colombet]

Hi Louis,

LGTM.

Small nitpicks:
+    if(abs(cast<ConstantSDNode>(V.getNode())->getSExtValue()) < 1<<(width-1))
+      return true;
+    else
+      return false;
One “else" slipped through :)

+  //SDNode *AndNode = SubsNode()->getOperand(0).getNode();
Not intended comment, I guess.

Please commit with those changes.

Thanks,
-Quentin
> On Aug 28, 2014, at 2:47 PM, Louis Gerbarg <lgg at apple.com> wrote:
> 
> I was actually really surprised the index were the same when I implemented it, but I decided not to question my luck. Attached is a new patch that incorporates your suggestions and attempt to clarify the bit of the explanation about the bit patterns used for testing.
> 
> Louis
> <0001-Remove-spurious-mask-operations-from-AArch64-add-com.patch>
>> On Aug 26, 2014, at 1:52 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>> 
>> Hi Louis,
>> 
>> I still have trouble understanding how you compute the magic values.
>> In particular, what are those 24 distinct bit patterns that can result from the extension of 4 bit values?
>> 
>> Here are a few nitpicks:
>> #1
>> +      return true;
>> +    } else
>> +      return false;
>> 
>> The else statement is useless after a return and must be removed.
>> 
>> #2
>> +// the exact values of AddConstant, SubConstant, and CC, along with the nominal
>> Typo: SubConstant -> CompConstant
>> 
>> #3
>> +  SDNode *AndNode = N->getOperand(3)->getOperand(0).getNode();
>> For readability, I would replace  N->getOperand(3) by SubsNode.
>> 
>> #4
>> +  if ((AddInputValue2.getNode()->getOpcode() != ISD::Constant &&
>> +       AddInputValue2.getNode()->getOpcode() != ISD::TargetConstant) ||
>> 
>> => !isa<ConstantSDNode>(AddInputValue2.getNode())
>> 
>> +      (SubsInputValue.getNode()->getOpcode() != ISD::Constant &&
>> +       SubsInputValue.getNode()->getOpcode() != ISD::TargetConstant))
>> 
>> => !isa<ConstantSDNode>(SubsInputValue.getNode())
>> 
>> #5
>> I would prefer to have the index of both the CCVal and the Cmp passed to performCONDCombine instead of having hard coded constant in that function.
>> Indeed, performCONDCombine is used with two different nodes: AArch64::CSEL and AArch64::BRCOND. It happens that both nodes have the CCVal and the Cmp respectively at index 2 and index 3. I am afraid that if we want to reuse this function on another node, we would miss that those values are expected to be at those indexes.
>> That is not a critical change as the problem will occur in an obvious manner. Feel free to fix or not. It was just weird to see that we passed two different node to that function and did not make any special cases to recognize one or the other, whereas they are very different.
>> 
>> Typos:
>> - without with a mask -> without and with a mask
>> - perscribed -> prescribed
>> - equivlent -> equivalent
>> - extendeding -> extending
>> +  // There is a SUBS feeding this condition. Is it fed by a mask we can
>> +  // use.
>> -> // use?
>> 
>> Thanks,
>> -Quentin
>> 
>> On Aug 25, 2014, at 3:46 PM, Louis Gerbarg <lgg at apple.com> wrote:
>> 
>>> Attached is a patch that should address your concerns. I provided a basic explanation of how the equations work and how they were derived. Not sure if I can go into much more depth without it being a bit over the top.
>>> 
>>> Louis
>>> <0001-Remove-spurious-mask-operations-from-AArch64-add-com.patch>
>>>> On Aug 22, 2014, at 12:07 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>> 
>>>> Hi Louis,
>>>> 
>>>> Thanks for working on this, I like the general direction of your patch.
>>>> 
>>>> I haven’t looked at the logic closely yet, but here are a couple of comments:
>>>> - LLVM comments are complete sentences, please use a period at the end of your comments.
>>>> 
>>>> - The following comment is not very helpful. Could you explain the voodoo you are using (like a comment before each case), and give a brief description of the function instead of a link? For instance, I would have expected an explanation of what the arguments are and how they are used.
>>>> // This function does a whole lot of voodoo to determine if the tests are
>>>> // equivalent without with a mask. See:
>>>> // http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-July/074444.html
>>>> 
>>>> - Any particular reason why ExtType is a pointer and not a reference?
>>>> bool checkValueWidth(SDValue V, unsigned width, ISD::LoadExtType *ExtType) {
>>>> 
>>>> - Use CHECK-LABEL to match function names in the test cases, e.g.,
>>>> ; CHECK: test8_8
>>>> 
>>>> Thanks,
>>>> -Quentin
>>>> 
>>>> On Aug 21, 2014, at 2:21 PM, Louis Gerbarg <lgg at apple.com> wrote:
>>>> 
>>>>> This is the patch I discussed a while back in http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-July/074444.html. It eliminates spurious ands on 8 and 16 bit operations that have been promoted to 32 bit that have no material impact on these test cases they are fed into. It remove them in several cases that have come up in testing. It is entirely possible there are missed cases due to a slightly differently configured DAG, but adding those should be simple if we have examples of them. This patch has been tested against LNT on AArch64 hardware and includes tests that were randomly generated using on device data as well as ones derives from actual bugs.
>>>>> 
>>>>> Louis
>>>>> <0001-Remove-spurious-mask-operations-from-AArch64-add-com.patch>_______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>> 
>>> 
>> 
>