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

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin marked 8 inline comments as done.
rtereshin added inline comments.


================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:68
 
-const TargetRegisterClass *
+inline const TargetRegisterClass *
 MachineRegisterInfo::constrainRegClass(unsigned Reg,
----------------
bogner wrote:
> 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.
> 
Agreed


================
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:
> 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.
All right, I'll update the doc comments instead


https://reviews.llvm.org/D42159