[PATCH] D114611: [AVR] Expand STDWSPQRr & STDSPQRr, approach #2

[Andrew Dona-Couch via Phabricator via llvm-commits]

couchand added a comment.

Thanks for your work on this @Patryk27.

I've tested the change set by cherry picking onto the Rust fork and compiling a suite of example programs.  All of my verification has passed without error.

I will continue to try to come up with a test case that exercises the potential issue I raised inline, but please do let me know if I've missed something there.  Thanks again!



================
Comment at: llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp:1221
+                              ? AVR::R29R28
+                              : AVR::R31R30);
+
----------------
Perhaps I'm missing something, but where is Z being initialized?  I'm happy with the use of Y (I'm fine trusting the comment in the old code that Y is guaranteed to hold a copy of SP, though maybe I shouldn't just trust that).

The old code initialized Z immediately prior to converting the pseudo stores into real stores.  I can't see the same guarantee being made here.  Do we know that Z still holds the adjusted frame pointer value?  At first glance it seems like intervening instructions could easily clobber it.


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

https://reviews.llvm.org/D114611