[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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117426