[PATCH] D126488: [RISCV] reorganize comments in getFrameIndexReference [nfc]

Tue May 31 08:42:51 PDT 2022

reames planned changes to this revision.
reames added a comment.

In D126488#3544664 <https://reviews.llvm.org/D126488#3544664>, @frasercrmck wrote:

> No, I think the FP path is indeed different. The offsetting code lines up with the comment, doesn't it? That's what I gathered when working on this recently, anyway - I don't have the ability to verify it right now. I don't think it's correct to remove that comment wholesale. The only thing coming to me right now is a comment somewhere (code, commit - sorry) saying we want to put RVV objects as far away from the frame register as possible, hence the scalar/RVV swap when using FP. Or would that be better worded as "scalar as //close// to the frame register as possible"?

Thank you!  Taking another much closer look here, I think you are correct.  We do appear to actually change the relative order of the regions of the stack based on whether we have FP or not.  That is "surprising" to say the least!

Thinking about it, I think this even vaguely makes sense.  By doing it this way, we remove the need to include the variable-but-known size of the RVV region in the offset calculations for local objects.

I do worry about the implications for tooling of having the change in stack layout.  Anyone know of anything that might break/complicate unpleasantly?

> I do think we might be lacking RVV+FP testing. That was something I was going to look into.

I suspect this as well.

Will adjust this patch to incorporate the changed model.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126488