asb added inline comments.
================
Comment at: lib/CodeGen/CGCall.cpp:1937
+ RetAttrs.addAttribute(llvm::Attribute::ZExt);
+ }
// FALL THROUGH
----------------
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?
================
Comment at: lib/CodeGen/TargetInfo.cpp:8824
+
+ // We must track the number of GPRs used in order to conform to the RISC-V
+ // ABI, as integer scalars passed in registers should have signext/zeroext
----------------
mgrang wrote:
> I observed that you use RISCV and RISC-V interchangeably in comments. This is
> not a problem, per se but can make this uniform if we want to be *really*
> particular about this :)
Slightly different contexts. 'RISC-V' is the proper name of the target but
'RISCV' is the spelling of the LLVM target implementation. For this file at
least, I think it makes sense.
================
Comment at: lib/CodeGen/TargetInfo.cpp:8858
+
+ uint64_t Size = getContext().getTypeSize(Ty);
+ uint64_t NeededAlign = getContext().getTypeAlign(Ty);
----------------
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.
================
Comment at: lib/CodeGen/TargetInfo.cpp:8913
+ }
+ return getNaturalAlignIndirect(Ty, /*ByVal=*/true);
+}
----------------
efriedma wrote:
> The spec says "Aggregates larger than 2✕XLEN bits are passed by reference and
> are replaced in the argument list with the address". That's not byval.
The LLVM backend lowers byval in that way. Given that at this point we have a
trivial data type (plain struct or similar) which is copied-by-value by C
semantics, the only difference is whether the generated IR indicates implicit
copying with 'byval' or relies on the caller explicitly doing the copy. For
some reason I thought 'byval' was preferred, but looking again it seems
uncommon to do it this way. I've changed it to false - thanks for spotting this.
https://reviews.llvm.org/D40023
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
