[PATCH] D75638: [Hexagon] Support for Linux/Musl ABI.

[Brian Cain via Phabricator via cfe-commits]

bcain added a comment.

In general, I recommend qualifying metric and threshold values with their associated units: bits, bytes, pages, etc.  That way it's easy to see where the unit conversions are happening and easy to see whether the logic is correct without reasoning about how the value changes among lines.

Constants specified by the ABI should be designated with names and units [unless maybe this conflicts w/llvm project naming conventions?]



================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:7592
+    uint64_t Size = getContext().getTypeSize(Ty);
+    if (Size <= 64)
+      HexagonAdjustRegsLeft(Size, RegsLeft);
----------------
This `64` should probably be named something like `HEXAGON_REGISTER_SIZE_BITS_MAX`?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:7613
+  if (HexagonAdjustRegsLeft(Size, RegsLeft))
+    Align = Size <= 32 ? 32 : 64;
+  if (Size <= Align) {
----------------
`32` here should be `HEXAGON_REGISTER_SIZE_BITS_MIN` maybe?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:7705
+  // Round up to the minimum stack alignment for varargs which is 4 bytes.
+  uint64_t Offset = llvm::alignTo(CGF.getContext().getTypeSize(Ty) / 8, 4);
+
----------------
I suggest:


```
uint64_t Offset = llvm::alignTo(CGF.getContext().getTypeSize(Ty) / CHAR_BIT, HEXAGON_MIN_STACK_ALIGN_BYTES);
```


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:7748
+                                                 QualType Ty) const {
+  int ArgSize = CGF.getContext().getTypeSize(Ty) / 8;
+
----------------


```
int ArgSizeBytes = CGF.getContext().getTypeSize(Ty) / CHAR_BIT;
if (ArgSizeBytes > HEXAGON_VA_REGISTER_LIMIT_BYTES)
    return EmitVAArgFromMemory(CGF, VAListAddr, Ty);
```



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75638/new/

https://reviews.llvm.org/D75638