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

[Ayke via Phabricator via llvm-commits]

aykevl 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!");
+    }
----------------
benshi001 wrote:
> arsenm wrote:
> > 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
> Yes. This is problemetic. I guess the Ayke's intention is to let non-const shift amounts fall into the default ordinary process.
> 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

How so? Non-constant shifts are expanded by an IR pass to constant shifts (in a loop), see llvm/lib/Target/AVR/AVRShiftExpand.cpp. Maybe there's a way to circumvent it but with some testing I couldn't come up with IR that hits this assert. But I'll change it to `report_fatal_error` in an update.

(I also have a local patch that removes the IR pass and instead handles non-constant shifts here).


================
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));
----------------
arsenm wrote:
> Why can't you handle everything here as a normal custom lowering? Why do you need the custom inserter?
I'm not sure what you mean? How can this be done in a different way?

If you mean building the entire thing in SelectionDAG, I think that would be a lot more complicated if not impossible because eventually I want to generate loops. I intend to introduce a later patch that lowers these shits to a loop if emitting the full sequence results in more instructions and `minsize` is set (avr-gcc does the same).


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


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1847
+    // Shift one to the left.
+    for (size_t i = 0; i < Regs.size(); i++) {
+      size_t Idx = Regs.size() - i - 1;
----------------
benshi001 wrote:
> aykevl wrote:
> > benshi001 wrote:
> > > Would it be better to be 
> > > ```
> > > for (ssize_t i = Regs.size() - 1; i >= 0; i--) {
> > >  ...
> > >  if (i == Regs.size() - 1) {
> > >    ...
> > >   } else {
> > >    ...
> > >   }
> > > }
> > > ```
> > Fixed locally.
> I saw you use captical loop variable `I` in your other patch, https://reviews.llvm.org/D140570, maybe it would be better to also use `I` instead of `i` here.
That was already fixed locally :)


================
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));
----------------
arsenm wrote:
> Can directly initialize (also could use std::array?)
Good idea, fixed.


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

https://reviews.llvm.org/D140569