[PATCH] D35319: LSE Atomics reorg - Part I

[Tim Northover via Phabricator via llvm-commits]

t.p.northover 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())
----------------
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.


================
Comment at: lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp:178
+
+    // ARM Errata D11904 05/12/2017:
+    // The Acquire semantics are present when (A == 1 && Rt != 11111).
----------------
No need to mention that, especially not with a wrong-endian date. It's part of the ARMv8.1a spec.


Repository:
  rL LLVM

https://reviews.llvm.org/D35319