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

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin marked 3 inline comments as done.
rtereshin 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
----------------
bogner wrote:
> 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.
Okay, thanks! Will remove it.


================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:68
 
-const TargetRegisterClass *
+inline const TargetRegisterClass *
 MachineRegisterInfo::constrainRegClass(unsigned Reg,
----------------
bogner wrote:
> Why explicitly mark this inline?
It's a non-public helper function only used in 2 places with the only purpose to not repeat `getRegClass(Reg)` call while falling back to the original version of `constrainRegClass`. As `constrainRegAttrs` intends to replace `constrainRegClass` in most of the places, I don't want to create even an appearance of any unnecessary performance overhead on passes that run after ISel, especially when GlobalISel is not used at all.


================
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.
----------------
bogner wrote:
> Should this advice about which functions to call be put in the doc comments somewhere instead/as well?
Hm, probably. I'm a bit reluctant to change comments / docs to functions I didn't change though, as it might be considered as "unrelated change". Also, it might be a little too invasive before we actually move everything to `constrainRegAttrs`, maybe I should make a couple of other machine passes runnable mid-GISel-pipeline, and make it all a separate patch.

What do you think?


================
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
----------------
bogner wrote:
> I don't think anything in this test references the IR, so we can probably remove this whole block for readability.
Oh, interesting. So if just some of the functions are not defined, `llc` fails with `function '<function name>' isn't defined in the provided LLVM IR`, but if the entire block is removed it works just fine.

Didn't know that, thanks! Will remove it.


================
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: '' }
----------------
bogner wrote:
> 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.
Great, thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D42159