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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai marked 5 inline comments as done.
nemanjai 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,
----------------
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.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:2962
+            (v8i16 (COPY_TO_REGCLASS (XXBRH (COPY_TO_REGCLASS $A, VSRC)), VRRC))>;
   def : Pat<(v4i32 (PPCxxreverse v4i32 :$A)),
             (v4i32 (XXBRW $A))>;
----------------
steven.zhang wrote:
> The PPCxxreverse has completely the same semantics of bswap. I will commit a follow up patch to clean up it, as it might stop us to leverage the combine rules for bswap that implemented in DAGCombiner.
Thanks.


================
Comment at: llvm/test/CodeGen/PowerPC/vec-bswap.ll:112
+; Function Attrs: argmemonly nounwind willreturn
+declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
+
----------------
steven.zhang wrote:
> It is never used in the IR. Please remove it together with the end pair.
Ah, good catch. I'll get rid of these.


================
Comment at: llvm/test/CodeGen/PowerPC/vec-bswap.ll:123
+
+attributes #0 = { nounwind }
----------------
steven.zhang wrote:
> The attribute is also not needed.
I generally keep it to avoid the CFI directives in the ASM output, but in this case I am not using the script to produce checks and can (and will) remove this.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70436