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

[lcvon via Phabricator via llvm-commits]

lcvon007 marked 3 inline comments as done.
lcvon007 added inline comments.


================
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))
----------------
craig.topper wrote:
> `return ((STI.hasStdExtC() || STI.hasStdExtZcf() || (STI.hasStdExtZce())) && (STI.getFLen() == 32)`
done.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:474
+    if ((STI.hasStdExtC() || STI.hasStdExtZcf() || (STI.hasStdExtZce())) &&
+        (STI.getFLen() == 32))
+      return true;
----------------
craig.topper wrote:
> 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?
Use XLen to replace FLen, thanks very much.


================
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;
----------------
craig.topper wrote:
> 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()`
Have used XLen <=64 to replace FLen <=64  and the reason I add this condition is that FLD/FSD can be compressed in RV32DC and RV64DC only, please help review. @craig.topper  thanks a lot.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158623