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

[Ivan Kulagin via Phabricator via llvm-commits]

ikulagin added a comment.

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?



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2135
 
+void DAGTypeLegalizer::ExpandIntRes_ABS(SDNode *N, SDValue &Lo, SDValue &Hi) {
+  SDLoc dl(N);
----------------
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.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:927
 
+SDValue VectorLegalizer::ExpandABS(SDValue Op) {
+  EVT VT = Op.getValueType();
----------------
shchenz wrote:
> same as above, need some testcases for this code change
I think, tests are arсhitecture-specific too.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2918
 
+  bool IsUnary = false;
+
----------------
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?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2981
+    case SPF_ABS:
+      Opc = VT.isInteger() ? ISD::ABS : ISD::FABS;
+      IsUnary = true;
----------------
spatel wrote:
> shchenz wrote:
> > I think matchSelectionPattern can only find integer abs for now. Maybe we should only consider ISD::ABS for now and add ISD::FABS when matchSelectionPattern supports fabs?
> This was discussed in PR37743. We already have an llvm.fabs intrinsic, so I don't see why matchSelectPattern() would ever need to match fabs(). 
> 
> matchSelectPattern() is used for common operations that do *not* have LLVM intrinsics, so they are implemented using 'select' IR.
> 
> The fabs code here should be removed from this patch.
Ok, I'll remove it.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2985
+    case SPF_NABS:
+      // TODO: What DAG should be produced?
+      break;
----------------
spatel wrote:
> I think SPF_NABS will be as described in the summary - sub(0, abs(X)).
> 
> Unless there is evidence that the negation gets separated from the ABS dag node and we're failing to produce 'nabs' codegen for targets that support that op? In that case, we might add a ISD::NABS node.
> 
> Either way, let's leave nabs for a follow-up patch.
Ok, I'll implement this in another patch later.


Repository:
  rL LLVM

https://reviews.llvm.org/D49837