[PATCH] D71013: [AIX] Allow vararg calls when all arguments reside in registers.

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm marked 3 inline comments as done.
cebowleratibm added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7034
+      if (!GPR2.isRegLoc() || GPR2.getValNo() != GPR1.getValNo())
+        continue;
+      assert(GPR2.needsCustom() && "A second custom GPR is expected");
----------------
hubert.reinterpretcast wrote:
> cebowleratibm wrote:
> > hubert.reinterpretcast wrote:
> > > cebowleratibm wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > What sort of situation leads to this?
> > > > The matching ValNo should be an assertion.  I should have written:
> > > > 
> > > >        // If no GPRs were left then there may be no more ArgLocs remaining.
> > > >       if (I + 1 == e)
> > > >         continue;
> > > >       CCValAssign &GPR2 = ArgLocs[++I];
> > > >       // If no GPRs were left then there may be non-reg ArgLocs remaining.
> > > >       if (!GPR2.isRegLoc()))
> > > >         continue;
> > > >       assert(GPR2.getValNo() == GPR1.getValNo() && GPR2.needsCustom() &&
> > > >              "A second custom GPR is expected!");
> > > > 
> > > If we observed one custom reg and we are dealing with the var arg case. My understanding is that, if we were unable to allocate a second GPR for the vararg case, the loop would have hit `report_fatal_error`.
> > Yes we should have emitted an error.  So the final assertion checks that contract.  If we see a RegLoc that follows the "GPR1" RegLoc, then we expect that to be the second custom reg and have the same ValNo as the first.
> I am trying to understand why we cannot assert that there is at least one ArgLoc remaining, that said ArgLoc is a RegLoc, //and// that said RegLoc is the second custom reg.
You're right.  Given that we've seen the first custom reg we know the call is vararg, which means the second custom reg must be present or we would have emitted an error.

I think the code goes back to this when we add memloc support, however, to keep the implementation on trunk clean I'll tidy this up.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:697
+  %1 = load double, double* @d1, align 8
+  call void (i32, ...) @test_vararg(i32 42, double %conv, i64 42, double %1)
+  ret void
----------------
hubert.reinterpretcast wrote:
> cebowleratibm wrote:
> > ZarkoCA wrote:
> > > Can one of these be set as a float? This way it's explicitly shown that floats and doubles are handled differently. 
> > We could, but there's no way to do so from C code.  You can do so for typed functions, but then you don't initialize the GPR and this is outside the scope of this patch.
> > 
> > We could write such a test in IR with understanding nothing in C/C++ will produce such IR.
> This case suggested by @ZarkoCA raises the question of how a 32-bit floating point value will be represented in a 64-bit GPR. What is the expected result? The equivalent case for XL generates a 64-bit GPR with zeroes in the high bits and the bits forming the 32-bit floating point value in the low bits.
I agree there is a question of how an f32 occupies a 64-bit register, though this issue is not specific to calling convention and I had considered it outside the scope of this patch.

I'm adding an assembly test that the GPRs can be initialized without going through memory on newer hardware.  I'll also add an assembly test for passing an f32 to a typed function and validate the instructions used to populate the register.


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

https://reviews.llvm.org/D71013