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

[Ivan Kulagin via Phabricator via llvm-commits]

ikulagin added a comment.

> 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?

It make sense. But we must take into account that patches should be applied together, because changes from the patch 1 requires the patch 2 for correct testing the x86 and the subsequent patches for the rest architectures.



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2135
 
+void DAGTypeLegalizer::ExpandIntRes_ABS(SDNode *N, SDValue &Lo, SDValue &Hi) {
+  SDLoc dl(N);
----------------
shchenz wrote:
> 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.
It is impossible to test only  "Expand" without account subsequent "DAGCombining" and other DAG optimizations during the instruction selection for different architectures. I think, that the testing of ABS node with the different scalar/vector types for every architecture is better. Otherwise, there will be a lot of similar tests for every architecture that estimate the same thing.


Repository:
  rL LLVM

https://reviews.llvm.org/D49837