[PATCH] D70176: [Codegen][ARM] Add addressing modes from masked loads and stores

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Thanks for the changes. I am happy with this patch.
I could do a suggestion about readability, for me this would greatly improve that (a diff on top of this diff) because this has all the bit twiddling in one place, with all the patterns easy to see, and then you don't need to read any of that in the rest of the code:

  diff --git a/llvm/include/llvm/CodeGen/TargetLowering.h b/llvm/include/llvm/CodeGen/TargetLowering.h
  index 36e7509..ef7730c 100644
  --- a/llvm/include/llvm/CodeGen/TargetLowering.h
  +++ b/llvm/include/llvm/CodeGen/TargetLowering.h
  @@ -1150,8 +1150,6 @@ public:
          getIndexedLoadAction(IdxMode, VT.getSimpleVT()) == Custom);
     }
   
     /// Return how the indexed store should be treated: either it is legal, needs
     /// to be promoted to a larger size, needs to be expanded to some other code
     /// sequence, or the target has a custom expander for it.
  @@ -1171,15 +1169,59 @@ public:
          getIndexedStoreAction(IdxMode, VT.getSimpleVT()) == Custom);
     }
   
  +  // The Action is a 16 bit value in the table, and we encode the action for
  +  // the different instructions in the follow upper/lower parts of these 2
  +  // bytes:
  +  //
  +  //          top         bottom
  +  //    15            |           0 bit
  +  //    --------------------------|
  +  //    |  ML  |  MS  |  L  |  S  |
  +  //    --------------------------|
  +  //
  +  // where:
  +  //   ML = indexed masked load
  +  //   MS = indexed masked store
  +  //   L  = indexed load
  +  //   S  = indexed store
  +  //
  +  void writeIndexedModeActionLowerBottomByte(unsigned Idx, unsigned IdxMode,
  +                                             uint16_t Action) {
  +    IndexedModeActions[Idx][IdxMode] &= ~0xf;
  +    IndexedModeActions[Idx][IdxMode] |= Action;
  +  }
  +  void writeIndexedModeActionUpperBottomByte(unsigned Idx, unsigned IdxMode,
  +                                             uint16_t Action) {
  +    IndexedModeActions[Idx][IdxMode] &= ~(0xf << 4);
  +    IndexedModeActions[Idx][IdxMode] |= Action << 4;
  +  }
  +  void writeIndexedModeActionLowerTopByte(unsigned Idx, unsigned IdxMode,
  +                                          uint16_t Action) {
  +    IndexedModeActions[Idx][IdxMode] &= ~(0xf << 8);
  +    IndexedModeActions[Idx][IdxMode] |= Action << 8;
  +  }
  +  void writeIndexedModeActionUpperTopByte(unsigned Idx, unsigned IdxMode,
  +                                          uint16_t Action) {
  +    IndexedModeActions[Idx][IdxMode] &= ~(0xf << 12);
  +    IndexedModeActions[Idx][IdxMode] |= Action << 12;
  +  }
  +  LegalizeAction getIndexedModeLowerTopByte(unsigned Ty, unsigned IdxMode) const {
  +    return (LegalizeAction)((IndexedModeActions[Ty][IdxMode] >> 8) & 0xf);
  +  }
  +  LegalizeAction getIndexedModeUpperTopByte(unsigned Ty, unsigned IdxMode) const {
  +    return (LegalizeAction)((IndexedModeActions[Ty][IdxMode] >> 12) & 0xf);
  +  }
  +  /*
  +     TODO: the others
  +  */
  +
     /// Return how the indexed load should be treated: either it is legal, needs
     /// to be promoted to a larger size, needs to be expanded to some other code
     /// sequence, or the target has a custom expander for it.
     LegalizeAction getIndexedMaskedLoadAction(unsigned IdxMode, MVT VT) const {
       assert(IdxMode < ISD::LAST_INDEXED_MODE && VT.isValid() &&
              "Table isn't big enough!");
  -    unsigned Ty = (unsigned)VT.SimpleTy;
  -    // Masked Load action are kept in the upper half of the top byte.
  -    return (LegalizeAction)((IndexedModeActions[Ty][IdxMode] >> 12) & 0xf);
  +    return getIndexedModeUpperTopByte(VT.SimpleTy, IdxMode);
     }
   
     /// Return true if the specified indexed load is legal on this target.
  @@ -1195,9 +1237,7 @@ public:
     LegalizeAction getIndexedMaskedStoreAction(unsigned IdxMode, MVT VT) const {
       assert(IdxMode < ISD::LAST_INDEXED_MODE && VT.isValid() &&
              "Table isn't big enough!");
  -    unsigned Ty = (unsigned)VT.SimpleTy;
  -    // Masked Store action are kept in the lower half of the top byte.
  -    return (LegalizeAction)((IndexedModeActions[Ty][IdxMode] >> 8) & 0xf);
  +    return getIndexedModeLowerTopByte(VT.SimpleTy, IdxMode);
     }
   
     /// Return true if the specified indexed load is legal on this target.
  @@ -2099,9 +2139,7 @@ protected:
       assert(VT.isValid() && IdxMode < ISD::LAST_INDEXED_MODE &&
              (unsigned)Action < 0xf && "Table isn't big enough!");
       // Load action are kept in the upper half of the bottom byte.
  -    IndexedModeActions[(unsigned)VT.SimpleTy][IdxMode] &= ~(0xf << 4);
  -    IndexedModeActions[(unsigned)VT.SimpleTy][IdxMode] |= ((uint16_t)Action)
  -                                                          << 4;
  +    writeIndexedModeActionUpperBottomByte(VT.SimpleTy, IdxMode, Action);
     }
   
     /// Indicate that the specified indexed store does or does not work with the
  @@ -2113,9 +2151,7 @@ protected:
                                LegalizeAction Action) {
       assert(VT.isValid() && IdxMode < ISD::LAST_INDEXED_MODE &&
              (unsigned)Action < 0xf && "Table isn't big enough!");
  -    // Store action are kept in the lower half of the bottom byte.
  -    IndexedModeActions[(unsigned)VT.SimpleTy][IdxMode] &= ~0xf;
  -    IndexedModeActions[(unsigned)VT.SimpleTy][IdxMode] |= ((uint16_t)Action);
  +    writeIndexedModeActionLowerBottomByte(VT.SimpleTy, IdxMode, Action);
     }
   
     /// Indicate that the specified indexed masked load does or does not work with
  @@ -2127,10 +2163,7 @@ protected:
                                     LegalizeAction Action) {
       assert(VT.isValid() && IdxMode < ISD::LAST_INDEXED_MODE &&
              (unsigned)Action < 0xf && "Table isn't big enough!");
  -    // Masked Load action are kept in the upper half of the top byte.
  -    IndexedModeActions[(unsigned)VT.SimpleTy][IdxMode] &= ~(0xf << 12);
  -    IndexedModeActions[(unsigned)VT.SimpleTy][IdxMode] |= ((uint16_t)Action)
  -                                                          << 12;
  +    writeIndexedModeActionUpperTopByte(VT.SimpleTy, IdxMode, Action);
     }
   
     /// Indicate that the specified indexed masked store does or does not work
  @@ -2142,10 +2175,7 @@ protected:
                                      LegalizeAction Action) {
       assert(VT.isValid() && IdxMode < ISD::LAST_INDEXED_MODE &&
              (unsigned)Action < 0xf && "Table isn't big enough!");
  -    // Masked Store action are kept in the lower half of the top byte.
  -    IndexedModeActions[(unsigned)VT.SimpleTy][IdxMode] &= ~(0xf << 8);
  -    IndexedModeActions[(unsigned)VT.SimpleTy][IdxMode] |= ((uint16_t)Action)
  -                                                          << 8;
  +    writeIndexedModeActionLowerTopByte(VT.SimpleTy, IdxMode, Action);
     }
   
     /// Indicate that the specified condition code is or isn't supported on the 


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

https://reviews.llvm.org/D70176