[PATCH] D70724: [PowerPC] Add Support for indirect calls on AIX.

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked 6 inline comments as done.
sfertile added a comment.

In D70724#1768155 <https://reviews.llvm.org/D70724#1768155>, @hubert.reinterpretcast wrote:

> Just noting here that the test does not actually enforce that r2 is saved before being written to. Similarly for the other tests.


and

In D70724#1768155 <https://reviews.llvm.org/D70724#1768155>, @hubert.reinterpretcast wrote:

> Just noting here that the test does not actually enforce that the input register is populated before this instruction. Similarly for the other tests.


The problem is I have 2 different pairs of instructions that are order dependent with respect to each other, and several other instructions that can be interspersed. I'm not sure if its best to use `CHECK-NEXT` and verify the order at the cost of becoming fragile to the changes in the scheduler, or if its best to use 'CHECK-DAG` and expect that lnt and bootstrap to fail in the case the scheduler ever violated the dependency. I'm not aware of a way to write the test that both verifies those dependencies and keeps the flexibility for the independent instructions.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5255
   // place of the environment pointer.
+  assert((!hasNest || !Subtarget.isAIXABI()) &&
+         "Nest parameter is not supported on AIX.");
----------------
ZarkoCA wrote:
> hubert.reinterpretcast wrote:
> > ZarkoCA wrote:
> > > I'm getting confused by this assert. Will it assert if a function has a nest parameter and it's not AIX? 
> > > Can it be rewritten to be: 
> > > ```
> > >  assert((hasNest && Subtarget.isAIXABI()) && "Nest parameter is not supported on AIX.");
> > > ```
> > > 
> > > Or am i misunderstanding something? 
> > > 
> > > 
> > > Can it be rewritten to be:
> > > ```
> > >  assert((hasNest && Subtarget.isAIXABI()) && "Nest parameter is not supported on AIX.");
> > > ```
> > 
> > That formulation will trigger the assert //unless// it is AIX and `hasNest` is `true`.
> You're right, I blanked on the the way the assert works.  I see now it's meant to enforce that a function doesn't have nest or if it does it's not on AIX.  Please ignore my comment, Sean. 
I can Demorgan it if you prefer: `!(hasNest &&  Subtarget.isAIXABI()))`.


================
Comment at: llvm/test/CodeGen/PowerPC/aix_indirect_call.ll:1
+; RUN: llc -verify-machineinstrs -mcpu=pwr4 -mattr=-altivec \
+; RUN:  -mtriple powerpc-ibm-aix-xcoff -stop-after=machine-cp < %s | \
----------------
ZarkoCA wrote:
> I forget whether PWR4 supports altivec,. Wondering if you need `-mattr=-altivec` here? 
> 
> I remember there was a discussion about different altivec codegen behaviour when you specify any -mcpu vs leaving it. 
I need the attribute or I hit an error in `CallLowering_AIX`.  The check for the error is overly general (just checking if the subtarget supports altivec, and not for any particular use of the altivec functionality). I would like to remove the error but that would need to be done in a seperate patch from this one.


================
Comment at: llvm/test/CodeGen/PowerPC/aix_indirect_call.ll:72
+; OBJ32:         94 21 ff c0  stwu 1, -64(1)
+; OBJ32-DAG:     80 83 00 00  lwz [[REG:[0-9]+]], 0(3)
+; OBJ32-DAG:     90 41 00 14  stw 2, 20(1)
----------------
hubert.reinterpretcast wrote:
> This only parameterizes the scratch register in the human readable form. Two of the hex digits are sensitive to the scratch register value. Similarly for the other test.
Good catch, I completely missed that. The only encoding I'm actually interested in testing are the encodings of the 2 instructions expanded from the new pseudo. I've removed the other encodings from the test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70724