[PATCH] D70436: [PowerPC] Add missing legalization for vector BSWAP

Mon Nov 25 18:27:01 PST 2019

steven.zhang added inline comments.

================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:2950
   // Vector Byte-Reverse H/W/D/Q Word
   def XXBRH : XX2_XT6_XO5_XB6<60,  7, 475, "xxbrh", vsrc, []>;
+  def XXBRW : XX2_XT6_XO5_XB6<60, 15, 475, "xxbrw", vsrc,
----------------
nemanjai wrote:
> steven.zhang wrote:
> > It is surprising to me that, vsrc didn't have v16i8 and v1i128. However, it has nothing to do with this patch.  It would be great if someone could educate me on this :)
> The initial VSX specification did not provide any operations for sub-word elements so allowing such vectors in `vsrc` is not useful. Even now, aside from a few specific operations (such as bswap here), there isn't a meaningful set of VSX sub-word operations.
I think we need to fix it, as I see lots of changes if add them. Especially when some instructions want to put the result into VSX register while we are still putting them into VSR and move it to VSX. It will have some performance benefit from what I see. I will commit a patch later. Thank you for the explanation.

================
Comment at: llvm/test/Transforms/LoopVectorize/PowerPC/vectorize-bswap.ll:4
+; RUN:   -force-target-max-vector-interleave=1 -mcpu=pwr9 < %s | FileCheck %s
+define dso_local void @test(i32* %Arr, i32 signext %Len) {
+; CHECK-LABEL: @test(
----------------
nemanjai wrote:
> steven.zhang wrote:
> > The dso_local here is not need i think.
> I agree that it is not needed, do you have a compelling reason to remove it though?
No... Just to make the case as simple as possible. Anyway, it depends on you.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70436/new/

https://reviews.llvm.org/D70436