[PATCH] D35319: LSE Atomics reorg - Part I

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added a comment.

This diff covers lots of different areas:

1. DeadRegisterDefinitions: seems like a reasonable improvement,
2. The instruction definitions: horrible on the surface, but a massive bug might justify them. It's completely unclear why they're necessary (especially as this patch contains no tests).
3. The scheduling: whatever, assuming you have inside knowledge of ThunderX.



================
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())
----------------
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.


================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:9501-9503
+let Predicates = [HasLSE], mayLoad = 1, mayStore = 1, hasSideEffects = 1 in
+class BaseSTOPEncoding<dag oops, dag iops, string asm, string operands,
+                       string cstr, list<dag> pattern>
----------------
Why are you replacing an InstAlias with a real instruction? That's architecturally incorrect and only really done in LLVM when there's no other choice (weird encoding issues and so on in ARM). 


================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:9581-9582
+
+class BaseNOPEncoding<dag oops, dag iops, string asm, string operands,
+                      string cstr, list<dag> pattern>
+  : I<oops, iops, asm, operands, cstr, pattern> {
----------------
What does this have to do with atomics?


Repository:
  rL LLVM

https://reviews.llvm.org/D35319