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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added a comment.

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

I am not against them in general even though I let alot of feedback on why we should remove most of them :) .If you think they are useful I am willing to work with you to refine them into something we are both happy with. I think the same kind of goals we have when writing code comments apply to test comments:

- Anything that can reasonably be made self documenting should be.

- There shouldn't be any redundant comments.

- Any comments we do add need to be completely clear and accurate.

I think the 3rd point is already covered, Its 1 and 2 that I'm pushing for.



================
Comment at: llvm/test/CodeGen/PowerPC/Frames-large.ll:29
+
+; Test verifies:
+; - Frame pointer register (r31) is saved/defined/restored when frame pointer
----------------
Xiangling_L wrote:
> 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.
> Thanks for mentioning this, one reason I added this verbose description is there are some disturbing instructions like subf, addic in the CHECK content.

But the comment doesn't address that. For example if I look at the ELF 32-bit no-frame-pointer test, I see th e `subf` instruction but the test description doesn't help me understand why. Adding the CHECK for it makes it look like it is significant to the test as well, but as far as I can tell its completely redundant.

```
# Build up the stack frame adjustment in register $r0
        lis 0, -1
        ori 0, 0, 32752

# Allocate new stack frame.
        stwux 1, 1, 0

# Why do we have this instruction? It doesn't set any condition register bits, and the result is killed next instruction. The test description doesn't help me understand this.
        subf 0, 0, 1

# Spill $r31 so we can use it to restore the original stack pointer. If $r0 is available to spill to ... why not just use it as the scratch register though ...
        mr 0, 31

# Get original stack pointer value from the backchain.
        lwz 31, 0(1)

# Calculate the address of the stack allocated object. Why is it 20 bytes off the stack-pointer? I'm not very familiar with PPC32, this is soemthing I'd like commented becuase its non-obvious.
        addi 3, 1, 20

# deallocate stack frame.
        mr 1, 31

# Restore non-volatile register we clobbered.
        mr 31, 0

# Return, end of test.
        blr
```


================
Comment at: llvm/test/CodeGen/PowerPC/Frames-large.ll:31
+; - Frame pointer register (r31) is saved/defined/restored when frame pointer
+;   elimination is disabled.
+; - Allocated area is referred by stack pointer with correct offset.
----------------
I think you already did a good job of documenting this through your filecheck prefixes 'FP' vs 'NO-FP'.


================
Comment at: llvm/test/CodeGen/PowerPC/Frames-large.ll:33
+; - Allocated area is referred by stack pointer with correct offset.
+; - Allocated area is at least 4-byte aligned in the case of int.
+; - Only one instance of dynamic stack allocation in this case, identified by
----------------
I think instead of these 2 lines it would better to have white space before and after the add instruction that calculates the address of the allocated object. You could add a comment at that instruction if you want to draw extra attention but if I don't already know what that instructions importance is I don't think this comment helps.


================
Comment at: llvm/test/CodeGen/PowerPC/Frames-large.ll:36
+;   the stdux used to update the back-chain link when allocated frame is large
+;   that we can not address it by a 16-bit signed integer.
 
----------------
There is useful information in here that I don't think we could 'self-document' as part of the test.

> Only one instance of dynamic stack allocation in this case
Documented through the check prefixes in every test except `PPC32-LINUX-NOFP` becuase you check-next everything no other st[dw]ux could sneak in before the return.

> identified by
> ;   the stdux used to update the back-chain link when allocated frame is large
> ;   that we can not address it by a 16-bit signed integer.

That stdux/stwux is the register + register equivalent form of the register + immediate stores we would normally use is helpful to the test. How it adjusts the stack pointer and maintains the backchain might be useful as well, and that the limit where we have to switch to r+r from r+imm is when it overflows a signed 16-bit value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72454