[PATCH] D69578: [AIX] Add support for lowering int, float and double formal arguments.

[Zarko Todorovski via Phabricator via llvm-commits]

ZarkoCA added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6705
+  case MVT::i32:
+  case MVT::i64:
+    return IsPPC64 ? &PPC::G8RCRegClass : &PPC::GPRCRegClass;
----------------
sfertile wrote:
> My understanding is that when targeting 32-bit codegen any i64 will be split into 2 i32s. If thats correct then we should  keep the i64 case separate, with an assert along the lines of `assert(IsPPC64 && "i64 should have been split for 32-bit codegen.");`.
> 
> 
I can change it to: 

```
  case MVT::i1:
  case MVT::i32:
    (!isPPC64)
    return &PPC::GPRCRegClass;
    LLVM_FALLTHROUGH;
  case MVT::i64:
    assert(IsPPC64 && "i64 should have been split for 32-bit codegen.");
    return &PPC::G8RCRegClass;
```
I think a defensive assert like you're suggesting is a good idea. it does complicate the switch a bit but that may be necessary? 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6812
+    report_fatal_error("QPX support is not supported on AIX.");
+  if (Subtarget.hasAltivec())
+    report_fatal_error("Altivec support is unimplemented on AIX.");
----------------
sfertile wrote:
> I believe this is breaking some of the existing AIX lit testing.
Thanks,  I missed those in my testing.  Adding -mattr=-altivec gets them to pass.  I can add that to the testcases and ask the author to review. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6841
+      if (ValVT == MVT::i1)
+        ArgValue = DAG.getNode(ISD::TRUNCATE, dl, MVT::i1, ArgValue);
+      // PPC64 passes i8, i16, and i32 values in i64 registers. Promote
----------------
sfertile wrote:
> Shouldn't we be doing the same thing `extendArgForPPC64` is doing, but with the extra generality that the zext/zext type should be i32 when targeting 32-bit codegen?
All other PPC targets look like they just truncate i1s. Adding a condition to check for any extension flags for all types, including i1s, led to some nodes missing.  I will investigate further, however. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69578