[PATCH] D126932: [RISCV] Reduce scalar load/store isel patterns to a single ComplexPattern. NFCI

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

This looks clearly better to me.  A couple small comments, but plan to LGTM after that.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:1866
+
+  if (Addr.getOpcode() == ISD::ADD) {
+    if (auto *FIN = dyn_cast<FrameIndexSDNode>(Addr.getOperand(0))) {
----------------
Minor: You can combine these two blocks if written as:
if (Addr.getOpcode() == ISD::ADD ||
    (Addr.getOpcode() == ISD::OR && isOrEquivalentToAdd(Addr.getNode()))


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:1909
+                                         SDValue &Offset) {
+  if (Addr.getOpcode() == ISD::ADD) {
+    if (auto *CN = dyn_cast<ConstantSDNode>(Addr.getOperand(1))) {
----------------
Same minor code structure comment as above.

Given we repeat this, maybe add a stripAndAccumulate12BitOffset helper?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:1920
+    // We might be able to treat this OR as an ADD.
+    if (auto *FIN = dyn_cast<FrameIndexSDNode>(Addr.getOperand(0))) {
+      if (auto *CN = dyn_cast<ConstantSDNode>(Addr.getOperand(1))) {
----------------
Why does this case need to be restricted to frame index?  I think you maybe copy pasted this and didn't adjust?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126932