[PATCH] D47511: [AMDGPU] Construct memory clauses before RA

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

Needs some tests with frame indexes, and with the d16 lo/hi operations with tied operands



================
Comment at: lib/Target/AMDGPU/SIFormMemoryClauses.cpp:32-36
+// Clauses longer then 15 instructions would overflow one of the counters
+// and stall. They can stall even earlier if there are outstanding counters.
+static cl::opt<unsigned>
+MaxClause("amdgpu-max-memory-clause", cl::Hidden, cl::init(15),
+          cl::desc("Maximum length of a memory clause, instructions"));
----------------
I thought this was a subtarget specific limit.

I apparently never committed D40578 which made the hazard recognizer aware of the subtarget specific sizes


================
Comment at: lib/Target/AMDGPU/SIFormMemoryClauses.cpp:41
+class SIFormMemoryClauses : public MachineFunctionPass {
+  typedef DenseMap<unsigned, std::pair<unsigned, LaneBitmask>> RegUse;
+
----------------
Should this be using the BitVector of RegUnits instead?


================
Comment at: lib/Target/AMDGPU/SIFormMemoryClauses.cpp:312
+  MaxVGPRs = TRI->getAllocatableSet(MF, &AMDGPU::VGPR_32RegClass).count();
+  MaxSGPRs = TRI->getAllocatableSet(MF, &AMDGPU::SGPR_32RegClass).count();
+
----------------
SReg_32_XM0_XEXEC?


================
Comment at: lib/Target/AMDGPU/SIFormMemoryClauses.cpp:363
+
+      for (auto &&R : Defs) {
+        forAllLanes(R.first, R.second.second, [&R, &B](unsigned SubReg) {
----------------
Why && (and for the rest)?


================
Comment at: lib/Target/AMDGPU/SIInstructions.td:548-550
+def SI_MEMORY_CLAUSE : SPseudoInstSI <
+  (outs), (ins variable_ops)
+>;
----------------
Why do you use this instead of using the generic TargetOpcode::BUNDLE?
I think trying to use a custom bundle instruction is going to cause issues. For example instruction size computation for this won't work properly, and any other code that expects to see BUNDLE


https://reviews.llvm.org/D47511