[PATCH] D49837: [SelectionDAG][X86] Handle unary SelectPatternFlavor for ABS case in SelectionDAGBuilder::visitSelect.

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

In https://reviews.llvm.org/D49837#1178105, @ikulagin wrote:

> In https://reviews.llvm.org/D49837#1177863, @shchenz wrote:
>
> > Please merge your code with the patch in https://reviews.llvm.org/D48754 and run related tests to check there is no failure.
>
>
> I merged my code with the latest commits and run tests for X86 arch. For X86 everything is OK.
>  Now, I plan to make tests for other architectures and test them.
>  Or should I leave this for follow-up patches?


I strongly suggest you run tests for all arch for this patch since your code does not only impact X86. 
When I did this task by myself, I found there is some case need to promote ISD::ABS for some type, because I hit assert "Do not know how to promote this operator!". You should also meets this issue if you run check for other arch.
Below is one case for your reference:

: 'RUN: at line 1';   /home/shchenz/llvm/llvm/llvm/build/bin/llc -verify-machineinstrs < /home/shchenz/llvm/llvm/llvm/test/CodeGen/PowerPC/2008-07-15-Bswap.ll

And I have another comment, maybe you can split this patch into two parts:
Patch 1:  contain first three parts in your description

  SelectionDAGBuilder::visitSelect handles the unary SelectPatternFlavor::SPF_ABS case to build ABS node.
  Expand-based legalization of integer result for the ABS nodes.
  Expand-based legalization of ABS vector operations.

Patch 2: handle X86 specific

What do you think?



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2135
 
+void DAGTypeLegalizer::ExpandIntRes_ABS(SDNode *N, SDValue &Lo, SDValue &Hi) {
+  SDLoc dl(N);
----------------
ikulagin wrote:
> shchenz wrote:
> > add some test for this function?
> I think for this function test cases are architecture-specific. For example, the X86 tests contain the iabs.ll tests with few types.
Sorry I don't understand what you mean by test cases are architecture-specific? 
Yes, I see there are some cases for abs already in X86. But these cases pass without this patch. IMO, ideally there should be test cases for every patch with functionality changing to show what's current patch's improvement or fixing compared with trunk.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2918
 
+  bool IsUnary = false;
+
----------------
ikulagin wrote:
> shchenz wrote:
> > I am a little confused for IsUnary since abs is not the only unary node.  changing to one related with abs?
> Yes, but the SelectPatternFlavor contains only ABS/NABS patterns relating to unary node. Should I rename it to IsUnaryAbs?
Yes, it would be more clear.


Repository:
  rL LLVM

https://reviews.llvm.org/D49837