[PATCH] D91358: [ARM][RegAlloc] Add t2LoopEndDec

Dave Green via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 30 04:19:58 PST 2020


dmgreen added inline comments.


================
Comment at: llvm/lib/CodeGen/PHIElimination.cpp:452
+             "Expected operand 0 to be a reg def!");
+      SrcRegDef->getOperand(0).setReg(IncomingReg);
+      continue;
----------------
bjope wrote:
> dmgreen wrote:
> > bjope wrote:
> > > This is only valid if there are no other uses of SrcReg (no anywhere else, and not in the current MBB that we are eliminating PHI nodes inside, such as another PHI node in MBB).
> > > 
> > > Haven't analyzed it more closely than that. But I don't know if it can be generally guaranteed that the def in an UnspillableTerminator only has a single use. Is such a rule guaranteed here (or even feasible)?
> > > 
> > Yeah. We are being very careful!  We are only using these for hardware loop instructions that we control the insertion of and are careful to only do the transform to produce the instruction (t2LoopEndDec in this case) when it has a single remaining user. CheckUsers in MVETPAndVPTOptimisations::MergeLoopEnd does that.
> > 
> > That's why this is very opt-in and it's trying to be careful to say "Use with care".
> My concern was that this is not inside the target specific part of the backend. So if some other target wants to use the concept of "unspillable terminators" then there must be some clear rules what to expect.
> 
> I think that having an assert that there is only one use would be appropriate here (to at least catch any violations to such a rule at this point where things would go wrong otherwise).
> 
> But maybe the MachineVerifier should help out detect problems if an unspillable terminator def has more than one use as well (to catch such problems already in a pass breaking the rule)?
> 
> And maybe we need to document what is expected related to "unspillable terminators" related to this restriction somewhere (not sure exactly where). 
> 
> I guess there could be lots of generic passes, but also target specific passes, that must adhere to such a rule if it is allowed to insert an unspillable terminator already at ISel. So I still don't see how to ensure that all passes between ISel and PHI-elmination are aware about that they aren't allowed to introduce new uses of the unspillable terminator def? Or how close to PHI-elimination must a pass introducing "unspillable terminators" be?
> 
I agree. I have added a check to the verifier and an assert in here. In our case we are adding these for hardware loop instructions soon before registry allocation, after all the other mir level optimization have happened. There's also some details about the instruction in isUnspillableTerminatorImpl.


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

https://reviews.llvm.org/D91358



More information about the llvm-commits mailing list