[PATCH] D76401: [PowerPC][AIX] ByVal formal argument support: single register.

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked an inline comment as done.
sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7067
+      const unsigned TrueSize = Flags.getByValSize();
+      const unsigned StackSize = roundToMultiple(TrueSize, PtrByteSize);
+
----------------
cebowleratibm wrote:
> I understand that you're likely preparing for the next patch, but I think it's simple enough to:
> 
> if (ByValSize > PtrByteSize) report_fatal_error
> 
> then skip the rounding and write the single register case.  You know the StackSize is always PtrByteSize.
Rounding the byval size up is the same whether it is smaller then a single reg, or arbitrarily large. Using PtrByte size and skipping the rounding seems rather artificial, I'd rather keep this as is.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7094
+      SDValue Store =
+          DAG.getStore(CopyFrom.getValue(1), dl, CopyFrom, FrameIndex,
+                       MachinePointerInfo::getFixedStack(MF, FI, 0));
----------------
cebowleratibm wrote:
> Does this store get elided by optimization when possible?
> 
> For example:
> struct S {int i;};
> int foo(S s) { return s.i; }
> 
> I assume we don't want to manifest the store to the stack.  It's odd to me because we don't manifest the store for the equivalent:
> 
> int foo(int i) { return i; }
No. Several of the lit test cases show where we DAG combine to extract the value from the register (see the 4-byte 32-bit test or the 8-byte 64-bit test cases) but we don't remove the dead store. We will need work that can generalize the optimization the DAG combines is doing for extracting the values from the register, and a pass to clean up the stores.

>  It's odd to me because we don't manifest the store for the equivalent:
> int foo(int i) { return i; }

These are seemingly equivalent in C/C++ source, but consider the IR we have to generate for the 2 cases and I think the difference become apparent: 

```
; Function Attrs: norecurse nounwind readonly
define i32 @foo(%struct.S* nocapture readonly byval(%struct.S) align 4 %s) local_unnamed_addr #0 {
entry:
  %i = getelementptr inbounds %struct.S, %struct.S* %s, i64 0, i32 0
  %0 = load i32, i32* %i, align 4, !tbaa !3
  ret i32 %0
}

; Function Attrs: norecurse nounwind readnone
define i32 @Bar(i32 returned %i) local_unnamed_addr #1 {
entry:
  ret i32 %i
}
```

The GEP and the load force us to store to the stack for the ByVal arg making the 2 functions that happen to be 'equivalent' in source to be non-equivalent in their IR representations. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76401