[PATCH] D81108: [AArch64] Fix ldst-opt of multiple disjunct subregs.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1296
+      // impacting other instructions we did not check. Bail out.
+      if (RegClass->HasDisjunctSubRegs) {
+        LLVM_DEBUG(
----------------
efriedma wrote:
> fhahn wrote:
> > efriedma wrote:
> > > The fact that this doesn't whitelist specific opcodes makes me suspicious this is overlooking other potential issues.  Some instructions with two outputs require that they aren't equal.  A call instruction modifies a fixed register.  Some instructions have restricted register classes.
> > I initially thought relying on `renamable` should be enough for the cases mentioned above. The code to pick registers should already only pick out of the most restrictive register class.  It seemed to work quite well in practice so far. The problem with the disjunct sub register case is that those are represented as a single register in the result, but with as individual register when accessed by their users.
> > 
> > But it would probably be safer to have a whitelist. Do you know if there's a convenient way to do so for machine instructions, other than providing a really long list of opcodes?
> > 
> > 
> Hmm, you're right, "renamable" should have the right meaning.
> 
> Maybe worth clarifying the rules on whether isRenamable() should return true for something like the AArch64 LDP, where the two results have to be placed in distinct registers.  I guess you could argue renaming two defs to the same register should never be legal anyway. even if the instructions set attaches some meaning to it.  But I'm not sure this pass handles it correctly, in that case.  (In particular, I'm concerned what would happen if one of the results of an ldp was marked "undef".)
> 
> That said, this check still seems a little fragile: it's depending on the specific structure of the AArch64 register file.  In particular, the fact that you can't overwrite a "subregister" without overwriting the whole register.  Probably worth explicitly noting.
> Maybe worth clarifying the rules on whether isRenamable() should return true for something like the AArch64 LDP, where the two results have to be placed in distinct registers. I guess you could argue renaming two defs to the same register should never be legal anyway. even if the instructions set attaches some meaning to it. But I'm not sure this pass handles it correctly, in that case. (In particular, I'm concerned what would happen if one of the results of an ldp was marked "undef".)

I think the way we choose rename registers prevents using a register that is defined by the same instruction we are renaming. I can see if I can add a test case for that.

> That said, this check still seems a little fragile: it's depending on the specific structure of the AArch64 register file. In particular, the fact that you can't overwrite a "subregister" without overwriting the whole register. Probably worth explicitly noting.

I agree it is a bit fragile unfortunately, as it is easy to miss subtleties of AArch64 MIR, but I guess the only way around that would be a whitelist. Would you be happy with the current approach (with an added note as you suggested?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81108