[PATCH] D17454: Insert two S_NOP instructions for every high level source statement.

[Matt Arsenault via llvm-commits]

arsenm added a comment.

This needs some tests


================
Comment at: lib/Target/AMDGPU/SIInsertNopsPass.cpp:23
@@ +22,3 @@
+#include "AMDGPU.h"
+#include "AMDGPUSubtarget.h"
+#include "SIInstrInfo.h"
----------------
I don't think this is needed

================
Comment at: lib/Target/AMDGPU/SIInsertNopsPass.cpp:48-49
@@ +47,4 @@
+
+INITIALIZE_PASS_BEGIN(SIInsertNops, DEBUG_TYPE, PASS_NAME, false, false)
+INITIALIZE_PASS_END(SIInsertNops, DEBUG_TYPE, PASS_NAME, false, false)
+
----------------
If you don't need to initialize dependencies, you can use INITIALIZE_PASS without _BEGIN/_END

================
Comment at: lib/Target/AMDGPU/SIInsertNopsPass.cpp:62
@@ +61,3 @@
+
+  DenseMap<unsigned, MachineBasicBlock::iterator> L2I;
+  for (auto MBB = MF.begin(); MBB != MF.end(); ++MBB) {
----------------
I think this needs a better name. LineToInst?

================
Comment at: lib/Target/AMDGPU/SIInsertNopsPass.cpp:65
@@ +64,3 @@
+    for (auto MI = MBB->begin(); MI != MBB->end(); ++MI) {
+      if (!MI->isDebugValue() && MI->getDebugLoc()) {
+        auto DL = MI->getDebugLoc();
----------------
Can you invert the condition and continue to reduce indentation

================
Comment at: lib/Target/AMDGPU/SIInsertNopsPass.cpp:70
@@ +69,3 @@
+        if (L2IEntry == L2I.end()) {
+          BuildMI(*MBB, *MI, DL, TII->get(AMDGPU::S_NOP)).addImm(0);
+          L2I.insert(std::make_pair(CL, MI));
----------------
addImm should be on next line

================
Comment at: lib/Target/AMDGPU/SIInsertNopsPass.cpp:78-81
@@ +77,6 @@
+  }
+  for (auto L2IEntry = L2I.begin(); L2IEntry != L2I.end(); ++L2IEntry) {
+    auto MIParent = L2IEntry->second->getParent();
+    auto DL = L2IEntry->second->getDebugLoc();
+    auto MI = L2IEntry->second; ++MI;
+    if (MI != MIParent->end()) {
----------------
I think the use of auto and second here is hard to follow. ->second should be assigned to a variable, and no auto

================
Comment at: lib/Target/AMDGPU/SIInsertNopsPass.cpp:81
@@ +80,3 @@
+    auto DL = L2IEntry->second->getDebugLoc();
+    auto MI = L2IEntry->second; ++MI;
+    if (MI != MIParent->end()) {
----------------
++MI should be new line

================
Comment at: lib/Target/AMDGPU/SIInsertNopsPass.cpp:86
@@ +85,3 @@
+  }
+  BuildMI(MF.front(), MF.front().front(), DebugLoc(), TII->get(AMDGPU::S_NOP))
+    .addImm(0);
----------------
I would prefer having an explicit MBB variable instead of using the multiple fronds here


http://reviews.llvm.org/D17454