[PATCH] D88663: [AArch64] Use TargetRegisterClass::hasSubClassEq in tryToFindRegisterToRename

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.


================
Comment at: llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir:260
 # CHECK-NEXT:    renamable $x8 = MRS 55840
-# CHECK-NEXT:    STPWi $w9, killed renamable $w8, killed renamable $x0, 0 :: (store 4)
+# CHECK-NEXT:    STPWi $w2, killed renamable $w8, killed renamable $x0, 0 :: (store 4)
 # CHECK-NEXT:    RET undef $lr
----------------
efriedma wrote:
> This seems wrong: we shouldn't skip renaming implicit defs.  Probably we should bail out of the transform if we see an implicit def we can't understand.
> 
> The test input is a little weird, though; I'm not sure it's legal to mark the result of the ORRWrs renamable given the relationship between the result and the implicit def.
> This seems wrong: we shouldn't skip renaming implicit defs. Probably we should bail out of the transform if we see an implicit def we can't understand.
>
> The test input is a little weird, though; I'm not sure it's legal to mark the result of the ORRWrs renamable given the relationship between the result and the implicit def.

I'm not very familiar with this so I'm not sure what the right thing to do is to be honest. The optimization could handle these implicit-defs before using `MachineInstr::getRegClassConstraint`, so I guess it's a question of whether the input was right in the first place or if it can be extended to not bail out for these operands.

I grepped on ORRWrs and found a few uses similar to this, although that doesn't necessarily means it valid I suppose.


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

https://reviews.llvm.org/D88663