[PATCH] D75863: [AIX] Implement by-val caller arguments in a single register

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm planned changes to this revision.
cebowleratibm marked 12 inline comments as done.
cebowleratibm added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7167
+      // ByValSize. For example: a 7 byte by-val arg requires 4, 2 and 1 byte
+      // loads.
+      SDValue RegVal;
----------------
sfertile wrote:
> Could we do a larger load if the alignment is greater then the size? For example if we have a size of 7 and a type alignment of 1, but the argument is an auto local and the compiler increase the alignment to 8. I don't suggest making any such changes to this patch if it allowed, but something to consider once we have everything working.
The size should be a multiple of the alignment.  We can do a single load if we can prove PtrByteSize bytes are accessible from the argument address.  The auto local case you mention is probably the one worth looking into but I agree it's beyond the scope of this patch to optimize this case.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7189
+        assert(LocVT.getSizeInBits() >= (Bytes * 8));
+        if (unsigned PaddingInBits = LocVT.getSizeInBits() - (Bytes * 8)) {
+          // By-val arguments are passed left-justfied in register.
----------------
sfertile wrote:
> Why the name `PaddingInBits`? Isn't this is the shift size  to justify the bytes just loaded, where does padding come in?
"Padding" was meant to indicate the number of zero extended bits from the load that need to be left shifted to get the loaded bits into the correct bit offset.  I agree the term "Padding" is misleading.  I'll rename to "NumSHLBits".


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-byval.ll:125
+; 32BIT-NEXT:  renamable $r[[REGADDR:[0-9]+]] = LWZtoc @gS3, $r2 :: (load 4 from got)
+; 32BIT-DAG:   renamable $r[[REG2BYTE:[0-9]+]] = LHZ 0, killed renamable $r[[REGADDR]] :: (load 2)
+; 32BIT-DAG:   renamable $r[[REG1BYTE:[0-9]+]] = LBZ 2, renamable $r[[REGADDR]] :: (load 1)
----------------
sfertile wrote:
> Can we drop the 'BYTE' part of REG2BYTE and REG1BYTE? 
I'll just use REG1, REG2 ... as per usual generic reg names.  The reader will need to chase down the contents.

I found it useful to name the regs by content as I tried to keep everything straight in my head, but I don't mind the more condensed form.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75863