[PATCH] D48950: [PowerPC] Improve codegen for vector loads using scalar_to_vector

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

There are a few minor nits, but aside from those this LGTM.
Also, please make note of the fact that we need to fix sequences such as `xxsldwi -> xxspltw` and `xxpermdi -> xxspltw` in `PPCMIPeephole.cpp`.



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:2987
+	// Improvement to 32bit / 64bit vector loads 
+  let AddedComplexity = 400 in { 
+    // LIWAX - used for sign extending i32 -> i64 
----------------
amyk wrote:
> nemanjai wrote:
> > This code is within a block with `let AddedComplexity = 400` already set. So no need to repeat that.
> It could be I mistakenly seen, but the last `let AddedComplexity` block I saw was on line 2645, 
> ```
> let AddedComplexity = 400, Predicates = [HasP9Vector] in {                        
>   // Extra patterns expanding to vector Extract Word/Insert Word                    
>   def : Pat<(v4i32 (int_ppc_vsx_xxinsertw v4i32:$A, v2i64:$B, imm:$IMM)),           
>             (v4i32 (XXINSERTW $A, $B, imm:$IMM))>;                                  
>   def : Pat<(v2i64 (int_ppc_vsx_xxextractuw v2i64:$A, imm:$IMM)),                   
>             (v2i64 (COPY_TO_REGCLASS (XXEXTRACTUW $A, imm:$IMM), VSRC))>;           
>   } // AddedComplexity = 400, HasP9Vector
> ```
> so wouldn't I still need it? 
I don't think you're following the scope correctly, but it doesn't really matter. It doesn't hurt to have it multiple times.

In fact, it would probably be good to completely reorganize this file since it is huge and super difficult to follow whether you're in scope of some block. But that's of course a project for a separate patch.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:891
+  let isCodeGenOnly = 1 in 
+  def XXSLDWIs : XX3Form_2s<60, 2,
+                       (outs vsrc:$XT), (ins vsfrc:$XA, u2imm:$SHW),
----------------
Nit: move this up to just below the definition of the base instruction (`XXSLDWI`).


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:2994
+    // LIWZX - This instruction will be emitted for i32, f32, and when 
+		//					zero-extending i32 to i64 (zext i32 -> i64).  
+    let Predicates = [IsLittleEndian] in { 
----------------
There is an extra space in the indentation. The `z` should line up under `T`.


================
Comment at: llvm/test/CodeGen/PowerPC/swaps-le-6.ll:1
-; RUN: llc -verify-machineinstrs -mcpu=pwr8 \
+; RUN: llc -verify-machineinstrs -mcpu=pwr8 -ppc-vsr-nums-as-vr -ppc-asm-full-reg-names \
 ; RUN:   -mtriple=powerpc64le-unknown-linux-gnu -O3 < %s | FileCheck %s
----------------
These were originally broken up onto multiple lines to avoid having lines that are too long. Now the lines are too long and they're broken up onto multiple lines. Please fix the column width.


https://reviews.llvm.org/D48950