[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!");
+    }
----------------
arsenm wrote:
> 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
Okay.
It's a `report_fatal_error` now so it won't silently miscompile here anymore. If it ever happens, it must be very rare because before this patch it would have resulted in a linker error (`__lshlsi3` etc) instead of a compiler error.


================
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:
> 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
Ok, understood. Next time I do an optimization that doesn't require flow control I'll try doing this in `XXXDAGToDAGISel::trySelect` (I assume that's what you mean?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140569