[PATCH] D138529: [AVR] Optimize constant 32-bit shifts

[Ayke via Phabricator via llvm-commits]

aykevl marked 4 inline comments as done.
aykevl added a comment.

Thank you for taking a look! I will update the patch later with your suggestions. For now, I've updated the patch a bit with changes I made locally in the past few days.
(I will also update the patch with extra context that I forgot to add this time).



================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:302
+        cast<ConstantSDNode>(N->getOperand(1))->getZExtValue();
+    SDValue Cnt = DAG.getTargetConstant(ShiftAmount, dl, MVT::i8);
+    unsigned Opc;
----------------
benshi001 wrote:
> It is OK that  `ShiftAmount` have a `MVT::i8` type ? Could it be better to `MVT::i16` ?
32-bit shift instructions will shift at most 31 bits, so i8 is good enough.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:314
+    case ISD::SRA:
+      Opc = AVRISD::ASRW;
+      break;
----------------
benshi001 wrote:
> Would it better to use `std::map` or equivalent but more efficient llvm utilities?
Do you know of any?
Other code seems to use a `switch` and I think this is very readable.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1866
+    Register Zero = MRI.createVirtualRegister(&AVR::GPR8RegClass);
+    BuildMI(*BB, MI, dl, TII.get(AVR::COPY), Zero).addReg(AVR::R1);
+
----------------
benshi001 wrote:
> How about using GetZeroRegister() instead of fixed AVR::R1? This way should also fit avrtiny. Or we can emit eor Rx, Rx instead of mov Rx, Zero .
This was already fixed in my local changes (this patch is older than D138582).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138529