[PATCH] D155829: [LoongArch] Add LSX intrinsic support

[WÁNG Xuěruì via Phabricator via llvm-commits]

xen0n added a comment.

Just took a quick look mainly checking for stylistic concerns and things look good at first glance! I'd like to do a closer review but I may not be able to adequately finish it before the LLVM 17 branching next week. I'll try though (to get more people looking here perhaps).



================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.h:64-75
+  // Vector Shuffle
+  VREPLVE,
+
+  // Extended vector element extraction
+  VPICK_SEXT_ELT,
+  VPICK_ZEXT_ELT,
+
----------------
Put those below the `Intrinsic operations start" marker perhaps?


================
Comment at: llvm/lib/Target/LoongArch/LoongArchLSXInstrInfo.td:20
+def loongarch_replve : SDNode<"LoongArchISD::VREPLVE", SDT_LoongArchVreplve>;
+def loongarch_all_non_zero : SDNode<"LoongArchISD::VALL_NONZERO",
+                                    SDT_LoongArchVecCond>;
----------------
Make the pattern name `loongarch_vall_nonzero` for consistency with the C++ name? Same for other similar cases.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchLSXInstrInfo.td:1526
+                "VILVL_B", "VILVH_B"] in
+  def : Pat<(!cast<Intrinsic>(!tolower("int_loongarch_lsx_"#Inst))
+               (v16i8 LSX128:$vj), (v16i8 LSX128:$vk)),
----------------
Can this `!tolower` call be lifted into a helper class like `deriveInsnMnemonic` at least? (I don't know if the whole `!cast<Intrinsic>` expression could be abstracted this way, or even the whole `Pat` def.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155829