[PATCH] D93687: [VE] Extract & insert vector element isel

[Kazushi Marukawa via Phabricator via llvm-commits]

kaz7 added inline comments.


================
Comment at: llvm/lib/Target/VE/VEISelLowering.cpp:293
 #include "VVPNodes.def"
+
+    setOperationAction(ISD::INSERT_VECTOR_ELT, LegalVecVT, Legal);
----------------
simoll wrote:
> kaz7 wrote:
> > The definition of BUILD_VECTOR action and other INSERT_VECTOR_ELT/EXTRACT_VECTOR_ELT actions are separated by VVP actions.
> You mean that the `setOperationAction` calls for VVP are not at the top or bottom of the `initVPUActions` function?
> If i understand correctly, why does it matter?
Yes.  If you have a certain reason to do that, it's fine.  But, at least, I would like to understand it since I may change codes also.  If you are doing without any reason, I would like to ask you to organize changes in maybe next patch.


================
Comment at: llvm/lib/Target/VE/VEInstrPatternsVec.td:27
+              (SuperRegCast $sy),
               i32:$vl)>;
 }
----------------
simoll wrote:
> kaz7 wrote:
> > You can merge these three lines.
> Great! I suppose you mean the `VBRDrl` and `VBRDil` patterns. How do i do this? There would need to be a `noop` replacement for the `ImmOp` Operand..
No.  What I said is merge three lines into one line like `(VBRDrl (SuperRegCast $sy) i32:$vl)>;`  Doing that organize `vbrd_elm32` and `vbrd_elm64` consitently and make review easy.


================
Comment at: llvm/test/CodeGen/VE/Vector/extract_elt.ll:35
+
+define fastcc i64 @extract_ri_v512i64(<512 x i64> %v) {
+; CHECK-LABEL: extract_ri_v512i64:
----------------
simoll wrote:
> kaz7 wrote:
> > I simply don't understand why this won't cause errors...
> Isel legalizes this into two `v256i64` arguments, which are mapped to `v0` and `v1` 
I see.  I was afraiding that compiler is generating wrong code without any warning/errors.  Now, I've understood why `lvs` uses `%v1(116)` as its operands.  Thanks.


================
Comment at: llvm/test/CodeGen/VE/Vector/extract_elt.ll:85
+
+define fastcc i32 @extract_rr_v512i32(<512 x i32> %v, i32 %idx) {
+; CHECK-LABEL: extract_rr_v512i32:
----------------
`i32 signext %idx`


================
Comment at: llvm/test/CodeGen/VE/Vector/extract_elt.ll:181
+
+define fastcc float @extract_rr_v512f32(<512 x float> %v, i32 %idx) {
+; CHECK-LABEL: extract_rr_v512f32:
----------------
`i32 signext %idx`


================
Comment at: llvm/test/CodeGen/VE/Vector/insert_elt.ll:46
+
+define fastcc <256 x i32> @insert_rr_v256i32(i32 signext %idx, i32 %s) {
+; CHECK-LABEL: insert_rr_v256i32:
----------------
`i32 signext %s`, or `i32 zeroext %s` to avoid `and`.


================
Comment at: llvm/test/CodeGen/VE/Vector/insert_elt.ll:56
+
+define fastcc <256 x i32> @insert_ri7_v256i32(i32 %s) {
+; CHECK-LABEL: insert_ri7_v256i32:
----------------
same here.


================
Comment at: llvm/test/CodeGen/VE/Vector/insert_elt.ll:66
+
+define fastcc <256 x i32> @insert_ri8_v256i32(i32 %s) {
+; CHECK-LABEL: insert_ri8_v256i32:
----------------
same here.


================
Comment at: llvm/test/CodeGen/VE/Vector/insert_elt.ll:77
+
+define fastcc <512 x i32> @insert_ri_v512i32(i32 %s) {
+; CHECK-LABEL: insert_ri_v512i32:
----------------
same here.


================
Comment at: llvm/test/CodeGen/VE/Vector/insert_elt.ll:91
+
+define fastcc <512 x i32> @insert_rr_v512i32(i32 signext %idx, i32 %s) {
+; CHECK-LABEL: insert_rr_v512i32:
----------------
same here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93687