[PATCH] D27195: [ARM] GlobalISel: Lower more than 4 arguments

[Diana Picus via Phabricator via llvm-commits]

rovka added inline comments.


================
Comment at: lib/Target/ARM/ARMCallLowering.cpp:51
+  void assignValueToReg(unsigned ValVReg, unsigned PhysReg,
+                        CCValAssign &VA) override {
+    assert(VA.isRegLoc() && "Value shouldn't be assigned to reg");
----------------
qcolombet wrote:
> t.p.northover wrote:
> > This is silently ignoring the signext/zeroext modifiers that are needed to correctly handle small types on AAPCS. It'd probably be better to fallback or assert if they're not being supported yet.
> I would recommend to change the ValueHandler APIs to return boolean so we can indeed fallback while stuff is not supported.
Right now we're falling back before reaching this point, by returning false from ARMCallLowering::lowerReturn if the type is not strictly 32-bit wide (see line 86). I think assertions would be more appropriate in this case, because it's not too complicated to check all the value types up front before calling this. I'll add some throughout.


================
Comment at: lib/Target/ARM/ARMInstructionSelector.cpp:84-86
+  using namespace TargetOpcode;
+  switch (I.getOpcode()) {
+  case G_ADD:
----------------
t.p.northover wrote:
> Is this selection stuff intentionally in the same patch? Seems like it's an orthogonal issue.
It's intentional, I wanted to have end-to-end support for more than 4 arguments, and that includes selecting loads from the stack. 


https://reviews.llvm.org/D27195