[PATCH] D35319: LSE Atomics reorg - Part I

[Christof Douma via Phabricator via llvm-commits]

christof added a comment.

Few inline comments on the dead register pass. I've not looked in detail to the other changes done in this patch.



================
Comment at: lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp:124
+      CASE_AARCH64_ATOMIC_(CASA):
+      CASE_AARCH64_ATOMIC_(CASAL):
+
----------------
I don't think CAS and CASP lose acquire semantics when they target the zero register. Am I wrong?


================
Comment at: lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp:141
+
+    for (unsigned I = 0; I < ND; ++I) {
+      const MachineOperand &MO = MI.getOperand(I);
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D35319