[PATCH] D94465: [RISCV] Frame handling for RISC-V V extension. (2nd. version)

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetFrameLowering.h:27
 namespace TargetStackID {
-  enum Value {
-    Default = 0,
----------------
Is this just a formatting change?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:162
+  DebugLoc DL = II->getDebugLoc();
+  int64_t NumOfVReg = Amount / 8;
+
----------------
Is Amount expected to be a multiple of 8 or does the division need to take the ceiling?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:167
+  Register FactorRegister = MRI.createVirtualRegister(&RISCV::GPRRegClass);
+  if (isPowerOf2_32(NumOfVReg)) {
+    uint32_t ShiftAmount = Log2_32(NumOfVReg);
----------------
Is Amount and NumOfVReg always positive? Should we check or assert that NumOfVReg fits in 32 bits before using isPowerOf2_32?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:171
+      return SizeOfVector;
+    else {
+      BuildMI(MBB, II, DL, TII->get(RISCV::SLLI), FactorRegister)
----------------
Drop this else, the if already returned.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:180
+        .addReg(RISCV::X0)
+        .addImm(NumOfVReg);
+    BuildMI(MBB, II, DL, TII->get(RISCV::MUL), FactorRegister)
----------------
Assert that NumOfVReg fits in 12 bits?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:216
 
-  if (!isInt<12>(Offset)) {
-    assert(isInt<32>(Offset) && "Int32 expected");
+  if (!isInt<12>(Offset.getFixed())) {
+    assert(isInt<32>(Offset.getFixed()) && "Int32 expected");
----------------
Is this if the same condition as the one at line 208? If so, how can this be true if we already signalled a fatal_error above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94465