[PATCH] D16239: Fix PR25526 by adding back limited cmpxchg pseudo-Insts

[Ahmed Bougacha via llvm-commits]

ab added a subscriber: ab.
ab added a comment.

This makes sense to me, but:

> Handle the expanded @llvm.aarch64.ldxr/stxr intrinsics to do away with any vregs (so fast-regalloc can't botch them), either in DAG or FastISel. I tried both, but the IR comparison always creates more. No change.


I don't understand: what "IR comparison"? If it worked, this seems to me like the ideal solution.


================
Comment at: lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:596-597
@@ +595,4 @@
+
+  MachineFunction *MF = MBB.getParent();
+  auto LoadCmpBB = MF->CreateMachineBasicBlock(MBB.getBasicBlock());
+  auto StoreBB = MF->CreateMachineBasicBlock(MBB.getBasicBlock());
----------------
MBB.addSuccessor(LoadCmpBB) ?

================
Comment at: lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:608
@@ +607,3 @@
+  LoadCmpBB->addLiveIn(Desired.getReg());
+  BuildMI(LoadCmpBB, MI.getDebugLoc(), TII->get(LdarOp), Dest.getReg())
+      .addReg(Addr.getReg());
----------------
Use a variable for MI.getDebugLoc(), perhaps?

================
Comment at: lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:632-633
@@ +631,4 @@
+
+  for (auto Reg : MBB.liveins())
+    DoneBB->addLiveIn(Reg);
+  DoneBB->splice(DoneBB->end(), &MBB, MI, MBB.end());
----------------
Is this sufficient? What happens to a def inside MBB used in DoneBB?

My MI-fu is weak, and I don't recall ever seeing code doing this when splicing MBBs (a quick grep confirms that), so I might be missing something obvious.

================
Comment at: lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:634
@@ +633,3 @@
+    DoneBB->addLiveIn(Reg);
+  DoneBB->splice(DoneBB->end(), &MBB, MI, MBB.end());
+
----------------
Should this also transferSuccessorsAndUpdatePHIs()?

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2328
@@ +2327,3 @@
+
+  MachineFunction &MF = CurDAG->getMachineFunction();
+  MachineSDNode::mmo_iterator MemOp = MF.allocateMemRefsArray(1);
----------------
MF is already available in SelectionDAGISel.

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:10049
@@ +10048,3 @@
+  // implement cmpxchg without spilling. If the address being exchanged is also
+  // on the stack and close enough to the, this can lead to a situation where
+  // the monitor always gets cleared and the atomic operation can never
----------------
missing word: "to the,"

================
Comment at: lib/Target/AArch64/AArch64InstrAtomics.td:380
@@ +379,3 @@
+
+let Constraints = "@earlyclobber $Rd, at earlyclobber $status" in {
+def CMP_SWAP_8 : Pseudo<(outs GPR32:$Rd, GPR32:$status),
----------------
mayLoad/mayStore here as well?


http://reviews.llvm.org/D16239