[PATCH] D40023: [RISCV] Implement ABI lowering

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/CodeGen/CGCall.cpp:1937
+        RetAttrs.addAttribute(llvm::Attribute::ZExt);
+    }
     // FALL THROUGH
----------------
asb wrote:
> rjmccall wrote:
> > asb wrote:
> > > rjmccall wrote:
> > > > I feel like a better design would be to record whether to do a sext or a zext in the ABIArgInfo.  Add getSignExtend and getZeroExtend static functions to ABIArgInfo and make getExtend a convenience function that takes a QualType and uses its signedness.
> > > I could see how that might be cleaner, but that's a larger refactoring that's going to touch a lot more code. Are you happy for this patch to stick with this more incremental change (applying the same sign-extension logic to return values as is used for arguments), and to leave your suggested refactoring for a future patch?
> > I won't insist that you do it, but I don't think this refactor would be as bad as you think.  Doing these refactors incrementally when we realize that the existing infrastructure is failing us in some way is how we make sure they actually happen.  Individual contributors rarely have any incentive to ever do that "future patch".
> I've submitted this refactoring in D41999.
Much appreciated, thanks!


https://reviews.llvm.org/D40023