[PATCH] D76360: [PPC][AIX] Emit correct Vaarg for 32BIT-AIX in clang

[Zarko Todorovski via Phabricator via cfe-commits]

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


================
Comment at: clang/test/CodeGen/ppc32-struct-return.c:53
+
+// AIX-SVR4: fatal error: error in backend: -msvr4-struct-return not supported on AIX
+ 
----------------
jasonliu wrote:
> jasonliu wrote:
> > sfertile wrote:
> > > jasonliu wrote:
> > > > If certain front end option is not supported on certain target, I think it makes more sense to have a standard diagnostic in the driver component, instead of "crash" in the backend. 
> > > > i.e. What if we specify this option on a Windows machine? Maybe we should pursue the same behavior. 
> > > Its not that we don't intend to support the option on AIX, but that support for the option takes further verification on AIX.  Since there is a difference how  AIX justifies subregister sized values in its argument passing, we can't just assume that this option will pack the return values the same way on AIX and Linux. 
> > > 
> > > The focus of this patch was originally to enable and verify the proper IR generation of va-arg/va-lis/va-start, however the scope of the patch has kept growing. If there are codegen differences in packing the return register with the svr4-return option enabled it will grow this patch once again. The fatal error lets us limit the scope of this patch, while not silently emiting bad codegen if there is a difference in how gcc initializes the return  registers. If during verification we decide we don't want to support the option on AIX, then adopting  a standard diagnostic in the driver component becomes the appropriate behavior. 
> > It wasn't clear for me from the code that this is not a permanent thing.  In that case, does it make sense to leave a TODO here and say we need to re-evaluate this in the future to decide if we want to support it or not on AIX?
> > On another note, I'm not sure if this is really needed on AIX target though. But I guess we could discuss it down the road. 
> Just to avoid ambiguity, I meant I'm not sure if we really need this *option* on AIX.
>The focus of this patch was originally to enable and verify the proper IR generation of va-arg/va-lis/va-start, however the scope of the patch has kept growing. If there are codegen differences in packing the return register with the svr4-return option enabled it will grow this patch once again. The fatal error lets us limit the scope of this patch, while not silently emiting bad codegen if there is a difference in how gcc initializes the return registers. If during verification we decide we don't want to support the option on AIX, then adopting a standard diagnostic in the driver component becomes the appropriate behavior.

@sfertile basically articulated my reasoning quite well here.  I just want to add, if verified to work on AIX, then we can simply remove the error in that one place and have the option working. 

GCC on AIX has those options and they work in the same way as described in https://reviews.llvm.org/D73290.  I think implementing a compatible GCC option in a fairly straightforward way doesn't hurt us.    

I agree with your point made earlier, I will add a TODO to make it clearer that we need to completely verify the behaviour. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76360