[PATCH] D29935: [RISCV 13/n] Codegen for conditional branches

[Dylan McKay via Phabricator via llvm-commits]

dylanmckay added a comment.

LGTM with minor error-handling nitpick



================
Comment at: lib/Target/RISCV/RISCVRegisterInfo.cpp:78
+  } else {
+    report_fatal_error(
+        "Frame offsets outside of the signed 12-bit range not supported");
----------------
asb wrote:
> dylanmckay wrote:
> > I think it makes more sense for `report_fatal_error` to live outside of the `else` entirely, and then the `else` can be removed as well.
> Just to clarify, are you suggesting the following?:
> ```
> if (!isInt<12>(Offset)) {
>   report_fatal_error(...)
> }
> 
> MI.getOperand(FIOperandNum).ChangeToRegister(FrameReg, false);
> MI.getOperand(FIOperandNum + 1).ChangeToImmediate(Offset);
> return;
> ```
> 
I was thinking something more like the following

```cpp
  if (isInt<12>(Offset)) {
    MI.getOperand(FIOperandNum).ChangeToRegister(FrameReg, false);
    MI.getOperand(FIOperandNum + 1).ChangeToImmediate(Offset);
  }

  report_fatal_error("Frame offsets outside of the signed 12-bit range not supported");
  return;
```

But now that you mention it, handling the error case early like your mockup seems better


https://reviews.llvm.org/D29935