[PATCH] D40023: [RISCV] Implement ABI lowering

Alex Bradbury via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Jan 12 05:53:49 PST 2018


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





More information about the cfe-commits mailing list