[PATCH] D72454: [AIX] Enable frame pointer for AIX and add related test suite

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L marked 7 inline comments as done.
Xiangling_L added inline comments.


================
Comment at: llvm/test/CodeGen/PowerPC/Frames-dyn-alloca-with-func-call.ll:29
+
+; Test verifies:
+; - Frame pointer register (r31) is saved/defined/restored.
----------------
sfertile wrote:
> I have a couple questions about this test.
> 
> > We actually have 9 instances of dynamic stack allocation, identified by the stdux used to update the back-chain link.
> Why 9 allocas? What is that exercising that say 2 or maybe 3 allocas wouldn't? The way this is worded it might imply that 9 dynamic allocations in the IR *must* produce 9 `stdux` instructions. Is that intended?
> 
> > Allocated area is 4 bytes aligned in the case of int.
> Where do we verify the stack adjustments are 4 byte aligned?  Would it be more appropriate to say 'is at least 4 byte aligned' instead?
1. `Why 9 allocas?`
Because we want to test when we pass more than 8 parameters to callee, how the stack pointer rather than frame pointer comes into play to save the 9th on the stack, which is `parameter save area are referred by stack pointer with correct offset`.

2.`The way this is worded it might imply that 9 dynamic allocations in the IR *must* produce 9 stdux instructions. Is that intended?`
Yes, it's intended.

3.`Would it be more appropriate to say 'is at least 4 byte aligned' instead?`
Yes, I agree, and I will update it.



================
Comment at: llvm/test/CodeGen/PowerPC/Frames-large.ll:29
+
+; Test verifies:
+; - Frame pointer register (r31) is saved/defined/restored when frame pointer
----------------
sfertile wrote:
> Does this test warrant such a detailed description? To me it seems obvious what we are testing: updating the stack frame when the adjustment is too large to be encoded in an immediate.
Thanks for mentioning this, one reason I added this verbose description is there are some disturbing instructions like `subf`, `addic` in the `CHECK` content. Those content is to add more integrity for the `CHECK`. But I would expect viewers to not focus too much on those by skimming through the description first.

Besides, I guess it depends on reviewers to think if this description is useful or not. AFAIK, @ZarkoCA and @cebowleratibm may prefer this. 

Anyway, I will leave it open for a while to see if there is some other input. Personally, I think it does no harm to have this description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72454