[PATCH] D42159: [GlobalISel] Making MachineCSE runnable in the middle of the GlobalISel

[Justin Bogner via Phabricator via llvm-commits]

bogner added inline comments.


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:666
 
+  /// constrainRegAttrs - Constrain the register class or the register bank of
+  /// the virtual register \p Reg to be a common subclass and a common bank
----------------
Don't repeat the function name in new doc comments. If the resulting inconsistency with the rest of the file bugs you you can feel free to clean up the others in a separate commit.


================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:68
 
-const TargetRegisterClass *
+inline const TargetRegisterClass *
 MachineRegisterInfo::constrainRegClass(unsigned Reg,
----------------
Why explicitly mark this inline?


================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:98-102
+  // A virtual register at any point must have either a low-level type
+  // or a class assigned, but not both. The only exception is the internals of
+  // GlobalISel's instruction selection pass, which is allowed to temporarily
+  // introduce registers with types and classes both. For that (and only for
+  // that) use RegisterBankInfo::constrainGenericRegister instead.
----------------
Should this advice about which functions to call be put in the doc comments somewhere instead/as well?


================
Comment at: test/CodeGen/AArch64/GlobalISel/machine-cse-mid-pipeline.mir:2-38
+--- |
+  target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
+  target triple = "aarch64-apple-ios"
+
+  define i32 @irtranslated() {
+  entry:
+    %a = add i32 1, 1
----------------
I don't think anything in this test references the IR, so we can probably remove this whole block for readability.


================
Comment at: test/CodeGen/AArch64/GlobalISel/machine-cse-mid-pipeline.mir:44-48
+registers:
+  - { id: 0, class: _, preferred-register: '' }
+  - { id: 1, class: _, preferred-register: '' }
+  - { id: 2, class: _, preferred-register: '' }
+  - { id: 3, class: _, preferred-register: '' }
----------------
Don't bother with the registers block (here and below). The constraints are specified inline in the IR anyway, so it isn't necessary unless you're specifying a preferred-register.


Repository:
  rL LLVM

https://reviews.llvm.org/D42159