[PATCH] D140569: [AVR] Custom lower 32-bit shift instructions

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:291
+      // 32-bit shifts are converted to a loop in IR.
+      llvm_unreachable("Expected a constant shift!");
+    }
----------------
aykevl wrote:
> benshi001 wrote:
> > `"Expected a constant shift amount!"`
> Fixed locally.
You can't guarantee this condition. If you don't want to handle this properly, should use emitError, or at least report_fatal_error and not unreachable. This could easily be run into by users


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:318
+    SDValue Result = DAG.getNode(Opc, dl, ResTys, SrcLo, SrcHi, Cnt);
+    return DAG.getNode(ISD::BUILD_PAIR, dl, MVT::i32, Result.getValue(0),
+                       Result.getValue(1));
----------------
Why can't you handle everything here as a normal custom lowering? Why do you need the custom inserter?


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1841
+  MachineRegisterInfo &MRI = BB->getParent()->getRegInfo();
+  DebugLoc dl = MI.getDebugLoc();
+
----------------
const ref


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1908-1912
+  SmallVector<std::pair<Register, int>, 4> Registers;
+  Registers.push_back(std::pair(MI.getOperand(3).getReg(), AVR::sub_hi));
+  Registers.push_back(std::pair(MI.getOperand(3).getReg(), AVR::sub_lo));
+  Registers.push_back(std::pair(MI.getOperand(2).getReg(), AVR::sub_hi));
+  Registers.push_back(std::pair(MI.getOperand(2).getReg(), AVR::sub_lo));
----------------
Can directly initialize (also could use std::array?)


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

https://reviews.llvm.org/D140569