[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:
> 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.









Repository:
  rL LLVM

https://reviews.llvm.org/D35319