[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
+
+    for (unsigned I = 0; I < ND; ++I) {
+      const MachineOperand &MO = MI.getOperand(I);
----------------
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.



Repository:
  rL LLVM

https://reviews.llvm.org/D35319