[PATCH] D44255: MIR-Canon tests and helper function additions.

[Justin Bogner via Phabricator via llvm-commits]

bogner added a comment.

I've commented on both parts, but please keep each review to a single topic. Adding the test is one thing, and adding the helpers should be another review (or better yet, just add them when you use them).



================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:102-106
 INITIALIZE_PASS_BEGIN(MIRCanonicalizer, "mir-canonicalizer",
-                      "Rename Register Operands Canonically", false, false)
+                      "Rename Register Operands Canonically", false, false);
 
 INITIALIZE_PASS_END(MIRCanonicalizer, "mir-canonicalizer",
+                    "Rename Register Operands Canonically", false, false);
----------------
These macros don't expand to expressions - adding semicolons doesn't make much sense.


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:134
 
-static bool rescheduleCanonically(MachineBasicBlock *MBB) {
+bool rescheduleLexographically(
+  std::vector<MachineInstr*> instructions,
----------------
Should this be static?


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:145-146
+    std::string s;
+    raw_string_ostream sstr(s);
+    II->print(sstr);
+
----------------
I think we usually put raw_string_ostream stuff in its own block to ensure it flushes or something. Please check other uses to see if this is right.


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:150
+    const size_t i = sstr.str().find("=");
+    auto Str = (i == std::string::npos) ? sstr.str() : sstr.str().substr(i);
+
----------------
Spelling out the type is clearer than auto here, though it might be simpler still to use StringRef::split or StringRef::take_until instead.


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:152
+
+    StringInstrMap.insert(StringMapEntry(Str, II));
+  }
----------------
I'm pretty sure we can do StringInstrMap.insert({Str, II}) and avoid the StringMapEntry typedef here. If not, std::make_pair is probably what we want.


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:173
+  MachineBasicBlock *MBB = MI->getParent();
+  for (auto BBI = MBB->instr_begin(), BBE = MBB->instr_end(); BBI != BBE; ++BBI)
+    if (&*BBI == MI)
----------------
Range-for probably cleans this up a bit.


================
Comment at: test/CodeGen/MIR/AArch64/mirCanonTest1.mir:14-35
+--- |
+  target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
+  target triple = "arm64-apple-ios11.0.0"
+
+  @IntGlob = common global double 0.000000e+00, align 8
+
+  define i32 @Proc8(double* %arg, [51 x double]* %arg1, double %arg2, double %arg3) #0 {
----------------
You should be able to remove the whole IR block here, it isn't really relevant to the test. You'll need to add -mtriple to the llc command.


================
Comment at: test/CodeGen/MIR/AArch64/mirCanonTest1.mir:38-67
+alignment:       2
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+registers:
----------------
I think you can get away with removing all of this metadata.


================
Comment at: test/CodeGen/MIR/AArch64/mirCanonTest1.mir:69
+stack:
+  - { id: 0, name: tmp, type: default, offset: 0, size: 4, alignment: 4,
+      stack-id: 0, callee-saved-register: '', callee-saved-restored: true,
----------------
You'll need to drop the names from these to get rid of the IR.


================
Comment at: test/CodeGen/MIR/AArch64/mirCanonTest1.mir:99
+    %0:gpr64common = COPY $x0
+    STRXui %0, %stack.1.tmp4, 0 :: (store 8 into %ir.tmp4)
+    STRXui %1, %stack.2.tmp5, 0 :: (store 8 into %ir.tmp5)
----------------
Without IR this will presumably read STRXui %0, %stack.1, 0 :: (store 8)


Repository:
  rL LLVM

https://reviews.llvm.org/D44255