[PATCH] D126465: [RISCV] Use knowledge of VLEN to avoid over-aligning the stack

[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:922
+    return std::make_pair(0, RVVStackAlign);
+  }
+
----------------
Everything above this line appears to be an NFC change + an early return.  Please split that, land, and rebase.

This is minor, but since I've now looked at this code several time when glancing at this patch, I'd rather not spend time glancing at it again.  


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:925
       ObjectSize = 8;
-    Offset = alignTo(Offset + ObjectSize, ObjectAlign);
     MFI.setObjectOffset(FI, -Offset);
----------------
This really doesn't look correct to me.

Given an object aligned to 32 bytes, knowing that it's a multiple of 16 bytes does not allow you to align it only to 2 bytes.  Which, unless I'm missing something, is what this code does.

I think what you need here is to compute the running alignment, use the MinVLen information to update that alignment, then if the required alignment is greater than that, adjust the offset.  

It's entirely possible I'm misreading this though.  Having a ObjectSize which is a integer for an object which is inherently variable sized doesn't make sense to me.  I'm not entirely sure what ObjectSize actually represents here.  Maybe there's just some missing comments in the original code?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126465