Remove redundant direct moves when extracting integers and converting to FP

[nemanjai]

Summary

Since we implemented extraction of integer vector elements using direct moves and we use direct moves for int to fp conversions, we end up with strange code when doing both operations. Namely, extracting an element and then converting that to fp results in a direct move out of a VSR followed immediately by a direct move back to a VSRL. This patch just eliminates both of those direct moves.
Power9 instructions won't help this situation so we would still have this issue with Power9 if we don't fix it with a patch like this.

A similar situation exists for extracting/converting integers of other widths, but the fix is non-trivial and it is not clear that there are alternatives that are much better. This can be addressed in the future.

This patch fixes PR 28117.

[amehsan]

This is how I think of this problem. Currently, when we encounter extract_element of a vector of integers, we generate shift and direct moves. That causes some inefficient patterns like the example for which I opened this bug.

I prefer to generalize the problem and, if possible, come up with a solution for the most general case, instead of focusing on this particular pattern. The more general problem that I can see is this: When we extract an element of an integer vector there are a number of possibilities.

1. all uses of the extracted element are in scalar integer operations.
2. The extracted element is going to be converted to a floating point and used in scalar or vector FP calculations.
3. The extracted element is going to be used in some subsequent vector integer calculations (for example inserted into a vector of int).
4. some mix of the above three cases.

Now I am not familiar with all ISD opcodes, so the rest of this comment, is missing some concrete details, but I hope my point is clear.

I think the right solution, will look at all uses of the extract_element and then based on those uses it will make sure the DAG has proper ISD opcodes so the subsequent pattern matching generates the right code.

For example if we only have (1) then we want the current pattern to be generated. If we have (1) and (2) then we need the direct move, but we don't want the users of type (2) to convert it back from integer to fp. I am not sure if (2) and (3) has to be treated as two different cases or not. The only difference here is register classes but probably at that stage we are still not aware of register classes.

I believe it should be possible to have DAG combine to massage the DAG depending on what kind of users extract_element has and then some simple and generic pattern matching will generate efficient code. Hopefully with this approach we do not need to hardcode many different patterns in the target description files.

If I am missing something about DAGCombine, or otherwise, that makes this approach non-practical, please let me know.

[amehsan]

It is also possible that we can fix this as a more general problem in DAGCombine (similar to what I said in the previous comment), but instead of having a extract-element-centric solution, we have some other kind of solution. For example something that looks at all converts in the DAG and removes unnecessary ones. So again, consider my previous comment as an example of a more general solution which might be practical. There might be better alternatives.

[nemanjai]

I am not sure I fully follow what you are suggesting. I understand that you are suggesting that we should look more generally into why we are extracting a vector element to begin with (i.e. what the uses are). However, I do not follow if you feel this patch should be abandoned in the interest of a more general solution (for which we haven't even fully formulated the problem).
In my view, extracting an integer element and converting it to floating point was an outlier in terms of code quality and it is what this patch fixes.

Now, I understand that there are some general opportunities for improvement with operations that convert back and forth between integer vector and scalar/vector floating point values due to the fact that some of the floating point and vector registers overlap and this is unique to PPC. This kind of solution requires considering the best choices for not only each of the uses you mention here, but also for both the source and result types. However, I don't think that a complex, fully generalized solution is likely to be implemented very soon and we should tackle these outliers in code quality in small chunks like this when we spot them. Also, we need to weigh the complexity of the solution against how common we feel the code pattern might be in real code.

So to summarize, while I definitely agree that extractelement, insertelement, scalar_to_vector and build_vector ISD nodes would benefit from a DAG combine to handle various possible uses of the results, I feel that such a solution is rather involved and needs a careful examination of what we want the resulting DAG's to look like. Until we have formulated such a solution, I think we should fix the obviously poor code gen choices with quick fixes such as this patch.

[amehsan]

this is fine with me if the alternative is complicated.

[kbarton]

LGTM

[nemanjai]

Committed revision 275796.

lib/Target/PowerPC/PPCInstrVSX.td

def : Pat<(int_ppc_vsx_xvredp v2f64:$A),
           (XVREDP $A)>;
 
 // Recip. square root estimate
 def : Pat<(int_ppc_vsx_xvrsqrtesp v4f32:$A),
           (XVRSQRTESP $A)>;
 def : Pat<(int_ppc_vsx_xvrsqrtedp v2f64:$A),
           (XVRSQRTEDP $A)>;
 
+let Predicates = [IsLittleEndian] in {
+def : Pat<(f64 (PPCfcfid (PPCmtvsra (i64 (vector_extract v2i64:$S, 0))))),
+          (f64 (XSCVSXDDP (COPY_TO_REGCLASS (XXPERMDI $S, $S, 2), VSFRC)))>;
+def : Pat<(f64 (PPCfcfid (PPCmtvsra (i64 (vector_extract v2i64:$S, 1))))),
+          (f64 (XSCVSXDDP (COPY_TO_REGCLASS (f64 (COPY_TO_REGCLASS $S, VSRC)), VSFRC)))>;
+def : Pat<(f64 (PPCfcfidu (PPCmtvsra (i64 (vector_extract v2i64:$S, 0))))),
+          (f64 (XSCVUXDDP (COPY_TO_REGCLASS (XXPERMDI $S, $S, 2), VSFRC)))>;
+def : Pat<(f64 (PPCfcfidu (PPCmtvsra (i64 (vector_extract v2i64:$S, 1))))),
+          (f64 (XSCVUXDDP (COPY_TO_REGCLASS (f64 (COPY_TO_REGCLASS $S, VSRC)), VSFRC)))>;
+} // IsLittleEndian
+
+let Predicates = [IsBigEndian] in {
+def : Pat<(f64 (PPCfcfid (PPCmtvsra (i64 (vector_extract v2i64:$S, 0))))),
+          (f64 (XSCVSXDDP (COPY_TO_REGCLASS $S, VSFRC)))>;
+def : Pat<(f64 (PPCfcfid (PPCmtvsra (i64 (vector_extract v2i64:$S, 1))))),
+          (f64 (XSCVSXDDP (COPY_TO_REGCLASS (XXPERMDI $S, $S, 2), VSFRC)))>;
+def : Pat<(f64 (PPCfcfidu (PPCmtvsra (i64 (vector_extract v2i64:$S, 0))))),
+          (f64 (XSCVUXDDP (COPY_TO_REGCLASS $S, VSFRC)))>;
+def : Pat<(f64 (PPCfcfidu (PPCmtvsra (i64 (vector_extract v2i64:$S, 1))))),
+          (f64 (XSCVUXDDP (COPY_TO_REGCLASS (XXPERMDI $S, $S, 2), VSFRC)))>;
+} // IsBigEndian
+
 } // AddedComplexity
 } // HasVSX
 
 // The following VSX instructions were introduced in Power ISA 2.07
 /* FIXME: if the operands are v2i64, these patterns will not match.
    we should define new patterns or otherwise match the same patterns
    when the elements are larger than i32.
 */
def XSCVUXDSP : XX2Form<60, 296,
                       [(set f32:$XT, (PPCfcfidus f64:$XB))]>;
 
   // Conversions between vector and scalar single precision
   def XSCVDPSPN : XX2Form<60, 267, (outs vsrc:$XT), (ins vssrc:$XB),
                           "xscvdpspn $XT, $XB", IIC_VecFP, []>;
   def XSCVSPDPN : XX2Form<60, 331, (outs vssrc:$XT), (ins vsrc:$XB),
                           "xscvspdpn $XT, $XB", IIC_VecFP, []>;
 
+  let Predicates = [IsLittleEndian] in {
+  def : Pat<(f32 (PPCfcfids (PPCmtvsra (i64 (vector_extract v2i64:$S, 0))))),
+            (f32 (XSCVSXDSP (COPY_TO_REGCLASS (XXPERMDI $S, $S, 2), VSFRC)))>;
+  def : Pat<(f32 (PPCfcfids (PPCmtvsra (i64 (vector_extract v2i64:$S, 1))))),
+            (f32 (XSCVSXDSP (COPY_TO_REGCLASS (f64 (COPY_TO_REGCLASS $S, VSRC)), VSFRC)))>;
+  def : Pat<(f32 (PPCfcfidus (PPCmtvsra (i64 (vector_extract v2i64:$S, 0))))),
+            (f32 (XSCVUXDSP (COPY_TO_REGCLASS (XXPERMDI $S, $S, 2), VSFRC)))>;
+  def : Pat<(f32 (PPCfcfidus (PPCmtvsra (i64 (vector_extract v2i64:$S, 1))))),
+            (f32 (XSCVUXDSP (COPY_TO_REGCLASS (f64 (COPY_TO_REGCLASS $S, VSRC)), VSFRC)))>;
+  }
+
+  let Predicates = [IsBigEndian] in {
+  def : Pat<(f32 (PPCfcfids (PPCmtvsra (i64 (vector_extract v2i64:$S, 0))))),
+            (f32 (XSCVSXDSP (COPY_TO_REGCLASS $S, VSFRC)))>;
+  def : Pat<(f32 (PPCfcfids (PPCmtvsra (i64 (vector_extract v2i64:$S, 1))))),
+            (f32 (XSCVSXDSP (COPY_TO_REGCLASS (XXPERMDI $S, $S, 2), VSFRC)))>;
+  def : Pat<(f32 (PPCfcfidus (PPCmtvsra (i64 (vector_extract v2i64:$S, 0))))),
+            (f32 (XSCVUXDSP (COPY_TO_REGCLASS $S, VSFRC)))>;
+  def : Pat<(f32 (PPCfcfidus (PPCmtvsra (i64 (vector_extract v2i64:$S, 1))))),
+            (f32 (XSCVUXDSP (COPY_TO_REGCLASS (XXPERMDI $S, $S, 2), VSFRC)))>;
+  }
 } // AddedComplexity = 400
 } // HasP8Vector
 
 let Predicates = [HasDirectMove] in {
   // VSX direct move instructions
   def MFVSRD : XX1_RS6_RD5_XO<31, 51, (outs g8rc:$rA), (ins vsfrc:$XT),
                               "mfvsrd $rA, $XT", IIC_VecGeneral,
                               [(set i64:$rA, (PPCmfvsr f64:$XT))]>,

test/CodeGen/PowerPC/remove-redundant-moves.ll

+; RUN: llc -mcpu=pwr8 -mtriple=powerpc64le-unknown-linux-gnu \
+; RUN:   -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -mcpu=pwr8 -mtriple=powerpc64-unknown-linux-gnu \
+; RUN:   -verify-machineinstrs < %s | FileCheck %s --check-prefix=CHECK-BE
+define double @test1(<2 x i64> %a) {
+entry:
+; CHECK-LABEL: test1
+; CHECK: xxswapd [[SW:[0-9]+]], 34
+; CHECK: xscvsxddp 1, [[SW]]
+; CHECK-BE-LABEL: test1
+; CHECK-BE: xxlor [[CP:[0-9]+]], 34, 34
+; CHECK-BE: xscvsxddp 1, [[CP]]
+  %0 = extractelement <2 x i64> %a, i32 0
+  %1 = sitofp i64 %0 to double
+  ret double %1
+}
+
+define double @test2(<2 x i64> %a) {
+entry:
+; CHECK-LABEL: test2
+; CHECK: xxlor [[CP:[0-9]+]], 34, 34
+; CHECK: xscvsxddp 1, [[CP]]
+; CHECK-BE-LABEL: test2
+; CHECK-BE: xxswapd [[SW:[0-9]+]], 34
+; CHECK-BE: xscvsxddp 1, [[SW]]
+  %0 = extractelement <2 x i64> %a, i32 1
+  %1 = sitofp i64 %0 to double
+  ret double %1
+}
+
+define float @test1f(<2 x i64> %a) {
+entry:
+; CHECK-LABEL: test1f
+; CHECK: xxswapd [[SW:[0-9]+]], 34
+; CHECK: xscvsxdsp 1, [[SW]]
+; CHECK-BE-LABEL: test1f
+; CHECK-BE: xxlor [[CP:[0-9]+]], 34, 34
+; CHECK-BE: xscvsxdsp 1, [[CP]]
+  %0 = extractelement <2 x i64> %a, i32 0
+  %1 = sitofp i64 %0 to float
+  ret float %1
+}
+
+define float @test2f(<2 x i64> %a) {
+entry:
+; CHECK-LABEL: test2f
+; CHECK: xxlor [[CP:[0-9]+]], 34, 34
+; CHECK: xscvsxdsp 1, [[CP]]
+; CHECK-BE-LABEL: test2f
+; CHECK-BE: xxswapd [[SW:[0-9]+]], 34
+; CHECK-BE: xscvsxdsp 1, [[SW]]
+  %0 = extractelement <2 x i64> %a, i32 1
+  %1 = sitofp i64 %0 to float
+  ret float %1
+}
+
+define double @test1u(<2 x i64> %a) {
+entry:
+; CHECK-LABEL: test1u
+; CHECK: xxswapd [[SW:[0-9]+]], 34
+; CHECK: xscvuxddp 1, [[SW]]
+; CHECK-BE-LABEL: test1u
+; CHECK-BE: xxlor [[CP:[0-9]+]], 34, 34
+; CHECK-BE: xscvuxddp 1, [[CP]]
+  %0 = extractelement <2 x i64> %a, i32 0
+  %1 = uitofp i64 %0 to double
+  ret double %1
+}
+
+define double @test2u(<2 x i64> %a) {
+entry:
+; CHECK-LABEL: test2u
+; CHECK: xxlor [[CP:[0-9]+]], 34, 34
+; CHECK: xscvuxddp 1, [[CP]]
+; CHECK-BE-LABEL: test2u
+; CHECK-BE: xxswapd [[SW:[0-9]+]], 34
+; CHECK-BE: xscvuxddp 1, [[SW]]
+  %0 = extractelement <2 x i64> %a, i32 1
+  %1 = uitofp i64 %0 to double
+  ret double %1
+}
+
+define float @test1fu(<2 x i64> %a) {
+entry:
+; CHECK-LABEL: test1fu
+; CHECK: xxswapd [[SW:[0-9]+]], 34
+; CHECK: xscvuxdsp 1, [[SW]]
+; CHECK-BE-LABEL: test1fu
+; CHECK-BE: xxlor [[CP:[0-9]+]], 34, 34
+; CHECK-BE: xscvuxdsp 1, [[CP]]
+  %0 = extractelement <2 x i64> %a, i32 0
+  %1 = uitofp i64 %0 to float
+  ret float %1
+}
+
+define float @test2fu(<2 x i64> %a) {
+entry:
+; CHECK-LABEL: test2fu
+; CHECK: xxlor [[CP:[0-9]+]], 34, 34
+; CHECK: xscvuxdsp 1, [[CP]]
+; CHECK-BE-LABEL: test2fu
+; CHECK-BE: xxswapd [[SW:[0-9]+]], 34
+; CHECK-BE: xscvuxdsp 1, [[SW]]
+  %0 = extractelement <2 x i64> %a, i32 1
+  %1 = uitofp i64 %0 to float
+  ret float %1
+}