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

[Ben Shi via Phabricator via llvm-commits]

benshi001 marked 4 inline comments as done.
benshi001 added inline comments.


================
Comment at: llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp:361-363
+  int ProgMemBank = AVR::getProgramMemoryBank(LD);
+  assert((0 <= ProgMemBank && ProgMemBank <= 5) &&
+         "unexpected program memory bank");
----------------
aykevl wrote:
> 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.
I also change the above `assert` to `report_fatal_error`.


================
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);
----------------
aykevl wrote:
> 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.
My previous intention is to say 

```
do not combine LDI into ELPM, since ELPM may be allocated a register in R0-R15, and LDI can not access.
```

Anyway, I change the comment to a more clear expression.


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

https://reviews.llvm.org/D116493