[PATCH] D117426: [AVR] Only push and clear R1 in interrupts when necessary

[Ben Shi via Phabricator via llvm-commits]

benshi001 added inline comments.


================
Comment at: llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp:460
+  // correctly zeroed in interrupts.
+  Ops.push_back(CurDAG->getRegister(AVR::R1, MVT::i8));
+
----------------
benshi001 wrote:
> benshi001 wrote:
> > benshi001 wrote:
> > > I am OK with current form. Just one more suggestion, you can add a new case of `icall`, current tests only contains a direct call but no indirect call. And you can add the `icall` test either in this patch or a new patch.
> > I have debugged your code, and I find this line is unnecessary.
> > 
> > Since there is already such a line in AVRISelLowering.cpp / LowerCall():
> > 
> > ```
> >   // Add argument registers to the end of the list so that they are known live
> >   // into the call.
> >   for (auto Reg : RegsToPass) {
> >     Ops.push_back(DAG.getRegister(Reg.first, Reg.second.getValueType()));
> >   }
> > 
> >   // The R1 register must be passed as an implicit register so that R1 is
> >   // correctly zeroed in interrupts.
> >   Ops.push_back(DAG.getRegister(AVR::R1, MVT::i8));
> > ```
> > 
> > That line in LowerCall is always executed, no matter direct call or indirect call, so the implicit `R1` will be added twice for indirect calls.
> I still accept your patch, and you can delete this line in AVRDAGToDAGISel::select<AVRISD::CALL> when commit.
@aykevl , do you have any different opinion on my comments? I suggest you commit this ASAP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117426