[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:
> > 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".


================
Comment at: lib/CodeGen/TargetInfo.cpp:8858
+
+  uint64_t Size = getContext().getTypeSize(Ty);
+  uint64_t NeededAlign = getContext().getTypeAlign(Ty);
----------------
asb wrote:
> rjmccall wrote:
> > I would encourage you to use CharUnits and getTypeSizeInChars more consistently in this code; it would simplify some of the conversions you're doing and eliminate some of the risk of forgetting a bits-to-bytes conversion.  It looks like storing XLen as a CharUnits would also make this easier.
> XLen is defined throughout the RISC-V ISA and ABI documentation as the width of the integer ('x') registers in bits. I've modified EmitVAArg to make use of CharUnits to avoid a conversion - there don't seem to be any other bit/byte conversions I can see.
Okay, I can accept that, thanks.


https://reviews.llvm.org/D40023