[PATCH] D158623: [RISCV] Reorder the stack frame objects.

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:470
+static bool isCompressibleLdOrSt(const MachineInstr &MI) {
+  const RISCVSubtarget &STI = MI.getMF()->getSubtarget<RISCVSubtarget>();
+  const unsigned Opcode = MI.getOpcode();
----------------
The usual structure for this type of thing is to switch over opcode, and put the predicates in the case blocks.  


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:491
+  const RISCVRegisterInfo *RI = STI.getRegisterInfo();
+  // It's only used to reduce codesize.
+  if (!MF.getFunction().hasOptSize())
----------------
I don't think this should be conditional on size optimization.  Using compressed loads and stores for frequently accessed objects should improve performance or at least be neutral.  


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:575
+
+    // If the two densities are equal, prioritize highest alignment
+    // objects. This allows for similar alignment objects
----------------
I think you're missing something really significant here.  If we start with

```
i32, align 4
i8, align 1
```

And we reorder to:

```
i8, align 1
i32, align 4
```

There's an extra three bytes of padding added between the objects.  This lengthens the total offset computation, and may push the second object out of the base+offset addressable range.  With a badly chosen set of objects, you can also end up growing the stack very significantly due to all the extra padding.

I would suggest that you start by restricting yourself to reordering objects of the same alignment.  (i.e. sort only within sets of object with equal alignment)


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:1505
+      // callee size.
+      if (STI.is64Bit() && CanCompress(496))
+        return 496;
----------------
This looks like a separate change, and should probably be it's own review with it's own tests.  

Also, magic constants are bad.  Why can't this be written in terms of StackAlign just like the non-compressed case just below?


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:583
+  for (auto &Obj : ObjectsToAllocate) {
+    SortingObjects[Obj].IsValid = true;
+    SortingObjects[Obj].ObjectIndex = Obj;
----------------
wangpc wrote:
> wangpc wrote:
> > Do we really need `IsValid`? It's always true I think (same for X86).
> OK, ignore it. `SortingObjects` is with bigger size than `ObjectsToAllocate`.
I think it would be much simpler to remove isValid and have a separate map from frame index to SortedObject index.  Please rework this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158623