[PATCH] D158623: [RISCV] Reorder the stack frame objects.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:467
+  case RISCV::SD:
+    if (STI.hasStdExtCOrZca() || STI.hasStdExtZce())
+      return true;
----------------
`return STI.hasStdExtCOrZca() || STI.hasStdExtZce()`


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:473
+    // C.FLW/C.FSW/C.FLWSP/C.SWSP is only supported by RV32FC
+    if ((STI.hasStdExtC() || STI.hasStdExtZcf() || (STI.hasStdExtZce())) &&
+        (STI.getFLen() == 32))
----------------
`return ((STI.hasStdExtC() || STI.hasStdExtZcf() || (STI.hasStdExtZce())) && (STI.getFLen() == 32)`


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:474
+    if ((STI.hasStdExtC() || STI.hasStdExtZcf() || (STI.hasStdExtZce())) &&
+        (STI.getFLen() == 32))
+      return true;
----------------
C.FLW should still be compressible with FLen==64 I think. It requires the F extension, but the D extension doesn't disable it.

Or was FLen here supposed to be XLen?


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:480
+    // C.FLD/C.FSD/C.FLDSP/C.FSDSP is only supported by RV32DC and RV64DC
+    if ((STI.hasStdExtC() || STI.hasStdExtZcd()) && STI.getFLen() <= 64)
+      return true;
----------------
Why STI.getFLen() <= 64? Shouldn't it be `== 64` or `>= 64`? Though if you see an FLD or FSD then FLEN must be at least 64 so you can probably ignore that check.

`return STI.hasStdExtC() || STI.hasStdExtZcd()`


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:486
+  }
+  return false;
+}
----------------
Make all the cases return and drop this return after the switch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158623