[PATCH] D20239: [Power9] Add codegen for VSX word insert/extract instructions

Ehsan Amiri via llvm-commits llvm-commits at lists.llvm.org
Fri May 20 12:55:33 PDT 2016


amehsan added inline comments.

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1500-1508
@@ -1493,1 +1499,11 @@
 
+/// isXXINSERTWMask - Return true if this VECTOR_SHUFFLE can be handled by
+/// the XXINSERTW instruction introduced in ISA 3.0. This is essentially any
+/// shuffle of v4f32/v4i32 vectors that just inserts one element from one vector
+/// into the other. This function will also set a couple of
+/// output parameters for how much the source vector needs to be shifted and
+/// what byte number needs to be specified for the instruction to put the
+/// element in the desired location of the target vector.
+bool PPC::isXXINSERTWMask(ShuffleVectorSDNode *N, unsigned &Op2Shift,
+                          unsigned &Op1Byte, bool &Swap, bool IsLE) {
+
----------------
I think people usually write comments for a member function in the header files and do not repeat the same comment in the implementation. It is natural to expect reader to go to header file if they are looking for comments, because for member variables and class, that is the only reasonable place to add a comment. 

My personal preference is not to replicate the comment twice. I'd like to see what others think about this.

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1507-1508
@@ +1506,4 @@
+/// element in the desired location of the target vector.
+bool PPC::isXXINSERTWMask(ShuffleVectorSDNode *N, unsigned &Op2Shift,
+                          unsigned &Op1Byte, bool &Swap, bool IsLE) {
+
----------------
amehsan wrote:
> I would rename this two variables: Op2Shift and Op1Byte, in a way that are consistent with each other. Because they are named very similarly, reader thinks that they should be interpreted in the same way. Op2Shift is easy to understand (Operand 2 of Shift instruction, may be Op2OfShift?) but the other one is hard to understand from the name. This should be what we want to choose as UIM in xxinsertw. is that correct? I think it will be easier to understand if the name reflects that.
It seems that Op1Byte and Op2Shift refer the vector operand of shuffle_vector. In that case shouldn't it be Op0 and Op1? Maybe you don't need to even mention Operand index in the variable name. Something like ShiftSize and InsertIndex, might work....


Repository:
  rL LLVM

http://reviews.llvm.org/D20239





More information about the llvm-commits mailing list