[AVR] Set mayLoad/mayStore flags of some load/store instructions #172986
Conversation
benshi001
requested review from
Patryk27,
XiaodongLoong,
aykevl and
jacquesguan
| : F16<0b1001010111001000, (outs), (ins), "lpm", []>, | ||
| Requires<[HasLPM]>; | ||
| let isReMaterializable = 1, mayLoad = 1, hasSideEffects = 0 in { | ||
| let Defs = [R0], Uses = [R31R30] in |
There was a problem hiding this comment.
Now that i think about it, why do we have Defs = [R0] here? 🤔
|
|
||
| // These pseudo instructions are combination of the OUT and ELPM instructions. | ||
| let Defs = [R0] in { | ||
| let Defs = [R0], hasSideEffects = 1 in { |
There was a problem hiding this comment.
ELPMBRdZ gets expanded to OUTARr + ELPMRdZ / ELPM that don't have hasSideEffects = 1 themselves, so this feels inconsistent 🤔
it seems that either those instructions should be marked as hasSideEffects or this instruction shouldn't have this annotation
There was a problem hiding this comment.
(i imagine OUTARr should be the one marked hasSideEffects = 1?)
| Uses = [R31R30] in def LPM | ||
| : F16<0b1001010111001000, (outs), (ins), "lpm", []>, | ||
| Requires<[HasLPM]>; | ||
| let isReMaterializable = 1, mayLoad = 1, hasSideEffects = 0 in { |
There was a problem hiding this comment.
Inside this block we've also got LPMRdZPi and LPMWRdZPi that I think are not rematerializable, because they increment the Z register.
| // | ||
| // This instruction may be removed once PR13375 is fixed. | ||
| let mayLoad = 1, hasSideEffects = 0 in | ||
| let hasSideEffects = 0 in |
There was a problem hiding this comment.
Since we're touching the code, it'd be nice to mark all instructions appropriately at once (add hasSideEffects = 0/1) - this seems to be what e.g. RISC-V is doing.
2 participants
fixes #156782