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

Florian Hahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 2 13:43:39 PST 2020


fhahn 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:
> c-rhodes wrote:
> > 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.
> Thinking about it a bit more, I guess you could argue the renamable marking is correct.  Just say that that if there's an overlapping implicit def, we have to follow some some target-specific rule to perform the rewrite.
> 
> The problem with rewriting implicit defs, in general, is that the target-independent instruction definition doesn't naturally encode any information about them.  If we're looking at ORRWrs specifically, sure, we can say that the implicit def corresponds to the result register.  But in general, it's an arbitrary register with unknown register allocation constraints.  Some can't be rewritten at all; for example, something like TLSDESC_CALLSEQ.  (Not sure we have those specifically this late, but there are a bunch of similar sorts of instructions.)
> 
> I think what should be happening here is that canRenameMOP() shouldn't accept arbitrary implciit defs.  If it's going to accept any, it needs to be ones where we know the specific rule required to rewrite them.
> 
> @fhahn Any thoughts?
> @fhahn Any thoughts?

Just coming back to this now, sorry! 

I think renaming implicit defs like in the `ORRWrs` case is quite valuable, because those are quite common.

Not sure how to best encode it, but teaching canRenameMOP about them might be the best way forward for now. I don't think there's any existing info that we can use.


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

https://reviews.llvm.org/D88663



More information about the llvm-commits mailing list