[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:
> > 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).
There's no verifier for this, so you still need to be more diligent about error handling. A DAG combine could still choose to introduce a non-constant shift amount, or any IR pass after the point you lowered it


================
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));
----------------
aykevl wrote:
> 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).
If you need to emit control flow, you don't have much choice besides the custom inserter


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

https://reviews.llvm.org/D140569