[PATCH] D86461: [AIX][XCOFF][Patch2] decode vector information and extent long table of the traceback table of the xcoff.

[Digger via Phabricator via llvm-commits]

DiggerLin marked 13 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:349-350
   // Masks to select leftmost bits for decoding parameter type information.
   static constexpr uint32_t ParmTypeIsFloatingBit = 0x8000'0000;
   static constexpr uint32_t ParmTypeFloatingIsDoubleBit = 0x4000'0000;
+  static constexpr uint32_t ParmTypeIsFixedBits = 0x0000'0000;
----------------
jasonliu wrote:
> I think with the two newly added fields, these two fields are not necessary. We just need to tweak `parseParmsType` a little bit so that it could uses the `ParmTypeIsFloatingBits` and `ParmTypeIsDoubleBits` below instead?
I will change the Macro as 
 static constexpr uint32_t ParmTypeIsFloatingBit = 0x8000'0000; 
-->
 static constexpr uint32_t ParmTypeIsFixedBit = 0x8000'0000;

and 

If I change function parseParmsType as 

static SmallString<32> parseParmsType(uint32_t Value, unsigned ParmsNum) {
  SmallString<32> ParmsType;
  for (unsigned I = 0; I < ParmsNum; ++I) {
    if (I != 0)
      ParmsType += ", ";
    if ((Value & TracebackTable::ParmTypeIsFixedBit) == 0) {
      // Fixed parameter type.
      ParmsType += "i";
      Value <<= 1;
    } else {
      switch (Value & TracebackTable::**ParmTypeMask**) {
         case TracebackTable::ParmTypeIsFloatingBits:
             ParmsType += "f";
         case  TracebackTable::ParmTypeIsDoubleBits
              ParmsType += "d";
      Value <<= 2;
    }
  }
  return ParmsType;
}

I do not think using Macro ParmTypeMask is a good idea hear.  for the parameter type without vector info , the ParmType is not always 2bits. 


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:401
+public:
+  TBVectorExt(uint16_t Data, uint32_t Vpi) : Data(Data), VecParmsInfo(Vpi) {}
+  uint8_t geNumOfVRSaved() const;
----------------
jasonliu wrote:
> If this constructor is only intended to get called by XCOFFTracebackTable, could we make the constructor private and mark XCOFFTracebackTable as a friend in this class?
OK, thanks



================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:407
+  bool hasVMXInstruction() const;
+  uint32_t getVecParmsInfo() const;
+  SmallString<32> getVectorParmsInfoString() const;
----------------
jasonliu wrote:
> Remove this declaration if it's not used.
thanks


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:424
+  Optional<TBVectorExt> VecExt;
+  Optional<uint8_t> XTBTable;
 
----------------
jasonliu wrote:
> The naming of `XTBTable` is not very self-explanatory here. 
> And I'm not sure if this field is actually useful, as this is really like a placeholder field. We don't really know what it is or what it does. 
> It's guaranteed later we will find out what it actually does, then we would need to update it to something else. So should we just skip this field?
I        unsigned char xtbtable; /* More tbtable fields, if longtbtable is set*/

I changed the name to ExtLongTBTable.



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:902
+static SmallString<32> parseParmsTypeWithVecInfo(uint32_t Value,
+                                                 unsigned int ParmsNum) {
+  SmallString<32> ParmsType;
----------------
jasonliu wrote:
> If we are going to separate it into two functions, why don't we also passing in the `number of vector parameters` as parameter here? So that the while loop could just check if the total number of parameter is reached instead of using `|| Value`. And you don't need the `bool Begin` as well.
the Vector parameter is not included in the ParmsNum. I think  we do not know the number of vector parameter untill we parse the VecExt = TBVectorExt(VRData, VecParmsInfo);


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1031
+      if (Cur)
+        VecExt = TBVectorExt(VRData, VecParmsInfo);
+    }
----------------
jasonliu wrote:
> I feel how we get the vector extension portion of the optional table is a bit wierd.
> This extension part is an all or nothing, but we takes two steps to get the data out of it.
> 
> Could we do something like 
> VecExt = TBVectorExt(DE.getBytes(Cur, TBVectorExtLength));
> 
> Then do some extra conversion inside of the constructor?
for the definition of 

struct vec_ext {
        unsigned vr_saved:6;    /* Number of non-volatile vector regs saved */
                                /* first register saved is assumed to be */
                                /* 32 - vr_saved                         */
        unsigned saves_vrsave:1;/* Set if vrsave is saved on the stack */
        unsigned has_varargs:1;
        unsigned vectorparms:7; /* number of vector parameters */
        unsigned vec_present:1; /* Set if routine performs vmx instructions */
        unsigned char vecparminfo[4];/* bitmask array for each vector parm in */
                                 /* order as found in the original parminfo, */
                                 /* describes the type of vector:            */
                                 /*       b'00 = vector char                 */
                                 /*       b'01 = vector short                */
                                 /*       b'10 = vector int                  */
                                 /*       b'11 = vector float                */
};

for the first 5 element of the structure  vec_ext are bit fields of 2 bytes data.
the vecparminfo  is 4 bytes.  If I do as  as your suggest , I still split the 6 bytes data into  uint16_t and uint32_t when parsing the data in the TBVectorExt. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86461