[PATCH] D65094: [PowerPC] Combine address computation to favour selecting DForm instructions

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added a comment.

The idea is great! Some comments.



================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5492
   SelectionDAG::allnodes_iterator Position = CurDAG->allnodes_end();
+  SmallVector<std::pair<const SDValue *, unsigned>, 16> DFormTracker;
 
----------------
I am not sure the intention that you select the vector instead of map.  The map make more sense to me.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5507
     }
+    if (ISD::isUNINDEXEDLoad(N) || ISD::isUNINDEXEDStore(N))
+      addMemOpToDFormTracker(N, DFormTracker);
----------------
Maybe, we need to respect the volatile and atomic flag ?


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5679
+unsigned
+PPCDAGToDAGISel::getBaseAndRequiredAlignmentFor(SDNode *MemOp,
+                                                const SDValue *&BasePtr) const {
----------------
bool getBaseAndRequiredAlignmentFor(SDNode *MemOp, const SDValue *&Base, unsigned &alignment) makes more sense.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5686
+  bool IsSExt = false;
+  if (Opc == ISD::LOAD) {
+    LoadSDNode *LD = cast<LoadSDNode>(MemOp);
----------------
There are some helper function to calculate the base and offset. i.e.
BaseIndexOffset::match(Store, DAG)


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5714
+    SDNode *MemOp,
+    SmallVectorImpl<std::pair<const SDValue *, unsigned>> &DFormTracker) {
+
----------------
The intention of addMemOpToDFormTracker is to collect the information and keep it in tracker. However, we are walking the tracker again later to get the uses of current base and discard the tracker. Why not generate the relationship between the base and offset here directly ? i.e.

maintain a map between:  Base -> { reqAlign, {BaseIndexOffset1, ... } } and BaseIndexOffset::match() will help us get the base and offset. Maybe, we need also keep the load/store pointer for each BaseIndexOffset as we will update it later.

Inside the updateMemOp..., walk the map one by one and handle it for each Base as what you did. 


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5739
+
+  int Offset = C->getZExtValue();
+  if (Offset % RequiredAlignment == 0 && isInt<16>(Offset))
----------------
uint64_t


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5746
+  if (!AddrValue->hasOneUse()) {
+    for (auto *UserOfAddr : AddrValue->getNode()->uses()) {
+      const SDValue *Dummy;
----------------
If we maintain the base map, we don't need to walk the uses as for each load/store, we will get the alignment and compare with the alignment in the map for each base.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5760
+  // save just one.
+  for (auto Elem : DFormTracker) {
+    if (*Elem.first == *AddrValue)
----------------
Don't need the loop if it is a map.


================
Comment at: test/CodeGen/PowerPC/unal-altivec-wint.ll:19
 ; CHECK-LABEL: @test1
-; CHECK: li [[REG:[0-9]+]], 16
 ; CHECK-NOT: li {{[0-9]+}}, 15
----------------
I didn't see the benefit for this change. Can we avoid it ?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65094