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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:922
+    return std::make_pair(0, RVVStackAlign);
+  }
+
----------------
reames wrote:
> 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.  
Landed separately, thanks for the suggestion.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:925
       ObjectSize = 8;
-    Offset = alignTo(Offset + ObjectSize, ObjectAlign);
     MFI.setObjectOffset(FI, -Offset);
----------------
reames wrote:
> 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?  
Right, that's basically what it does. I think it's correct.

The ObjectSize is the "known min size" portion of the scalable type size in bytes. The scalable portion is implicitly left for us (the target) to handle. We're artificially lowering the scalable parts of the offsets knowing that they'll all later be multiplied by `VLENB`. Since we're in control of the vscale multiplying, as long as all scalable objects are consistently treated in this manner, it should work out correct.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:896
+  // alignment. We can take this into account to avoid over-aligning the stack.
+  // Since VLEN is always a power of two greater than 32, knowing the minimum
+  // VLEN is enough to ensure the same alignment with larger VLENs.
----------------
frasercrmck wrote:
> craig.topper wrote:
> > VLEN can be 32. But our calculation of vscale and scalable types is broken for it.
> Ah yes, thanks - slip of the fingers. I wanted to say greater than //or equal to// 32. It's probably not worth a caveat here that we don't properly support 32 here, is it? It's largely irrelevant since the calculation works for any power of two.
(finally) fixed this typo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126465