[PATCH] D116493: [AVR] Generate ELPM for loading byte/word from extended program memory

[Ayke via Phabricator via llvm-commits]

aykevl added a comment.
Herald added a subscriber: jacquesguan.

This looks pretty good. Thanks for the improvements!

Can you add a test where you read twice from the same extended address space? It should only have one `ldi` instruction to load the address space number.

We should also save RAMPZ in interrupts on devices that support ELPM. But I guess that can happen in a separate patch.
See for example: https://godbolt.org/z/x9Tx1jzMc



================
Comment at: llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp:833
+        .addImm(STI.getIORegRAMPZ())
+        .addReg(BankReg, RegState::Kill);
+  }
----------------
I don't think this necessarily kills the register. A later instruction might use the same value.


================
Comment at: llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp:894
+      .addImm(STI.getIORegRAMPZ())
+      .addReg(BankReg, RegState::Kill);
+
----------------
Same here.


================
Comment at: llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp:361-363
+  int ProgMemBank = AVR::getProgramMemoryBank(LD);
+  assert((0 <= ProgMemBank && ProgMemBank <= 5) &&
+         "unexpected program memory bank");
----------------
I think it is better to use `report_fatal_error` here instead of `assert`. Asserts are removed in release builds, so this check will not be performed in release builds, while I think it should always be checked.

The same is true for the assert above (`Subtarget->hasLPM()`) but that is outside this patch.

In general, asserts should only be used for things that are always true unless there are bugs in the code. In this case, it is possible to trigger the assert without a bug in the AVR backend.


================
Comment at: llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp:384-386
+      // Do not combine the LDI instruction into the ELPM pseudo instruction,
+      // since LDI requires the destination register in range R16~R31.
+      SDValue NC = CurDAG->getTargetConstant(ProgMemBank, DL, MVT::i8);
----------------
I don't really understand this comment?
The instruction selector should always use a register class that is supported by all the instruction operands for the virtual register. In this case, that would be LD8.


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

https://reviews.llvm.org/D116493