[PATCH] D35319: LSE Atomics reorg - Part I

[Stefan Teleman via Phabricator via llvm-commits]

steleman added inline comments.


================
Comment at: lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp:141-142
+
+    for (unsigned I = 0; I < ND; ++I) {
+      const MachineOperand &MO = MI.getOperand(I);
+      if (!MO.isReg() || !MO.isDef())
----------------
christof wrote:
> steleman wrote:
> > t.p.northover wrote:
> > > steleman wrote:
> > > > steleman wrote:
> > > > > t.p.northover wrote:
> > > > > > steleman wrote:
> > > > > > > christof wrote:
> > > > > > > > t.p.northover wrote:
> > > > > > > > > steleman wrote:
> > > > > > > > > > t.p.northover wrote:
> > > > > > > > > > > This loop seems too generic: we know what instructions we're looking at, and we know that the entire purpose is to replace a register with XZR/WZR -- the surrounding code isn't going to do that if it's not in that class.
> > > > > > > > > > I do not understand what you are saying here.
> > > > > > > > > > 
> > > > > > > > > > The whole purpose of this function is to **prevent** the use of WZR/XZR, and **not** replace some other register with XZR/WZR, which would be wrong, as per ARM Errata.
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > Yes, but all of these instructions do have some definition that would otherwise be eligible. And if they didn't then the function wouldn't be necessary in the first place because the main loop checks.
> > > > > > > > Any particular reason why not to look only at the target (Wt) operand? Only the zero register as target operand makes the instructions ignore the acquire behaviour.
> > > > > > > The main loop checks //after// the ShouldSkip function is called, and not before it.
> > > > > > > 
> > > > > > > Therefore, the ineligible instructions have not already been excluded by the main loop, because the main loop hasn't executed yet.
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > Why does when it's checked matter? Also, even without that the loop is still redundant on the grounds that all of these instructions *do* have a dodgy class.
> > > > > > Also, even without that the loop is still redundant on the grounds that all of these instructions *do* have a dodgy class.
> > > > > 
> > > > > I don't understand what this means.
> > > > > 
> > > > > ShouldSkip needs to iterate over all the definitions of the MCInstrDesc, as to obtain the TargetRegisterClass:
> > > > > 
> > > > > 
> > > > > ```
> > > > > const MCInstrDesc &Desc = MI.getDesc();
> > > > > unsigned ND = Desc.getNumDefs();
> > > > > 
> > > > > // [ ... ]
> > > > > 
> > > > > const TargetRegisterClass *RC = TII->getRegClass(Desc, I, TRI, MF);
> > > > > if (RC == nullptr)
> > > > >   continue;
> > > > > 
> > > > > if (RC->contains(AArch64::WZR) || RC->contains(AArch64::XZR))
> > > > >   return true;
> > > > > 
> > > > > // [ ... ]
> > > > > ```
> > > > > 
> > > > > Does it not?
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > No reason - just that it was easier to write this way.
> > > > 
> > > > But looking only at the Wt operand is a very good suggestion, so I will change it thusly.
> > > > 
> > > > I don't understand what this means.
> > > 
> > > There is no instance in which this loop (once reached) will not return true, and that is guaranteed not just by dynamic register classes but by the structure of the instructions being considered (i.e. the MCInstrDescs are generated at compile time).
> > > 
> > > And just to keep banging on about the other point so it's not forgotten: even if it did return false because no such operand was present, the later code wouldn't try to replace the register anyway because it also checks whether XZR/WZR is valid.
> > > 
> > > The loop does nothing useful, and if it did that should be tested (since it's an addition rather than pure refactoring).
> > > 
> > > 
> > > There is no instance in which this loop (once reached) will not return true
> > 
> > OK, now I am really lost.
> > 
> > Are you saying that this (inner) loop:
> > 
> > 
> > ```
> > for (unsigned I = 0; I < ND; ++I) {
> > // [ ... ]
> > }
> > ```
> > 
> > will always return true?
> > 
> > 
> > 
> Yes. If you run through all the operands of the instructions listed earlier, at some point it will hit the operand for Wt. That operand has a known register class which includes the zero register.
Ahh, I just realized my mistake. So the TargetRegisterClass will always contain WZR/XZR because it contains **all** the registers used by the AArch64 Target, and //not// just the registers being used by this particular instruction.

What I am really looking for is the set of registers being used by this particular instruction.





Repository:
  rL LLVM

https://reviews.llvm.org/D35319