[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10323
+               bool EABI)
+      : DefaultABIInfo(CGT), XLen(XLen), FLen(FLen) {
+    if (EABI)
----------------
I think it'd be better to have a `NumArgGPRs(EAABI ? 6 : 8)` here as having a default value that gets overwritten is more error-prone (and harder to follow).


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2338
+// the ILP32E ABI.
+static const MCPhysReg ArgGPRs_NonE[] = {RISCV::X10, RISCV::X11, RISCV::X12,
+                                         RISCV::X13, RISCV::X14, RISCV::X15,
----------------
Underscores with camel-case isn't great. Maybe ArgIGPRs and ArgEGPRs or similar?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:104
+  // so that we don't insert `fp` manipulation code into functions that do not
+  // require it.
+  const MachineFrameInfo &MFI = MF.getFrameInfo();
----------------
Shouldn't this all be done by the generic stack realignment code like any other allocation? Or is the issue because it's _register spills_ not explicit allocas?


================
Comment at: llvm/test/CodeGen/RISCV/stack-realignment.ll:3
 ; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
-; RUN:   | FileCheck %s -check-prefix=RV32I
+; RUN:   | FileCheck %s -check-prefixes=RV32I,RV32-ILP32
+; RUN: llc -mtriple=riscv32 -target-abi ilp32e -verify-machineinstrs < %s \
----------------
Multiple prefixes is a bad idea with update_llc_test_checks.py, and why is this one done differently from the rest?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70401