[PATCH] D109069: [AArch64] Avoid adding duplicate implicit operands when expanding pseudo insts.

[Dave Green via Phabricator via llvm-commits]

dmgreen accepted this revision.
dmgreen added a comment.
This revision is now accepted and ready to land.

In D109069#2979191 <https://reviews.llvm.org/D109069#2979191>, @wwei wrote:

> Yes, we need to call transferImpOps here, because it will copy the implict operand include the flag--like `dead` flag.

The kill/dead flags only need to be conservatively correct, as far as I understand. So we can remove them and let another pass re-calculate them as needed. Having them more correct is usually a good thing though.

In D109069#2981752 <https://reviews.llvm.org/D109069#2981752>, @foad wrote:

> I guess addPhysRegDeps is not prepared to see two defs of the same physical register with inconsistent "dead" flags. Is it even valid MIR to have two defs of the same phys reg?

I would think it was invalid, yeah. At least in this case there doesn't seem to be much reason to keep both around.

It feels a little odd to me to bypass the BuildMI call so that we can copy the implicit defs from another instruction. As opposed to using/updating the ones that are added for this instruction. But from what I can tell they should be fine for all the instructions handled here, and I don't see anywhere else using transferImpOps that should have the same issues.

LGTM. Thanks for the fix.


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

https://reviews.llvm.org/D109069