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

[Zarko Todorovski via Phabricator via llvm-commits]

ZarkoCA added inline comments.


================
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.");
----------------
sfertile wrote:
> 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()))`.
It's fine the way it is.  I think it's more informative as to what triggers it in your original form. 


================
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 | \
----------------
sfertile wrote:
> 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.
I agree this is something we should discuss.  This error to may need to be added to lot of testcases in the future and, in turn, potentially removed when altivec is supported. While also potentially having to rewrite the tests. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70724