[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: lib/CodeGen/MachineRegisterInfo.cpp:68
 
-const TargetRegisterClass *
+inline const TargetRegisterClass *
 MachineRegisterInfo::constrainRegClass(unsigned Reg,
----------------
rtereshin wrote:
> 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.
Sure. It might be clearer to just make this a static function (you'll have to pass in the MachineRegisterInfo, obviously). Then the compiler will see that it's only used twice and do the inlining without the hint.



================
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.
----------------
rtereshin wrote:
> 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?
I don't think updating doc comments of existing functions to reflect how they relate to a new function is unrelated at all - I'd actually argue that it makes the reason for the change in wording more clear.


================
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
----------------
rtereshin wrote:
> 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.
Yep, if there's no function block at all the parser will make up synthetic functions.


https://reviews.llvm.org/D42159