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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:51
 
-  /// Set the target node this edge connects to.
-  void setTargetNode(const NodeType &N) { TargetNode = N; }
----------------
Unrelated change.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6807
+
+SDValue PPCTargetLowering::truncateScalarIntegerArg(ISD::ArgFlagsTy Flags,
+                                                    EVT ValVT,
----------------
Does it need to be a member of `PPCTargetLowering`? It looks like we are passing in everything we use. If we can reduce its scope to a static in `PPCISelLowering.cpp` I would prefer that.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6849
+    report_fatal_error("QPX support is not supported on AIX.");
+  if (Subtarget.hasAltivec())
+    report_fatal_error("Altivec support is unimplemented on AIX.");
----------------
Reporting an error for an unsupported construct is fine, but this ends up being very intrusive since we infer altivec support for pretty much any cpu we would target with AIX. We now have to add -mattr=-altivec on every llc invocation targteing AIX which is not very convenient.  If we compare it to the other fatal erros we have here: 
QPX, softFloat --> Will never be supported for AIX and we should error if those options are enabled.
isVarArg --> will trigger only if lowering a variadic function.
hasAltivec --> is inferred by the backend for any cpu we target, will be true even if we don't have any vector constructs.

The 'hasAltivec' restriction is simply too  restrictive. Instead we should only issue an error there is a use of a vector as an argument.  `CC_AIX` does that already I belive so we can omit any further error handling here.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6881
+            truncateScalarIntegerArg(Flags, ValVT, DAG, ArgValue, LocVT, dl);
+        ArgValue = DAG.getCopyFromReg(Chain, dl, VReg, LocVT);
+      }
----------------
Why the extra CopyFromReg?


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

https://reviews.llvm.org/D69578