[PATCH] D42565: [ARM][GISel] PR35965 Constrain RegClasses of nested instructions built from Dst Pattern

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/Utils.cpp:56-59
+  // Some of the instructions, like COPY, may not impose any register class
+  // constraints on some of their operands:
+  if (!RegClass)
+    return Reg;
----------------
qcolombet wrote:
> rtereshin wrote:
> > dsanders wrote:
> > > This is a bit more general than I'd like. I agree that TargetOpcodes like COPY shouldn't fail, but I think GenericOpcodes and target specific instructions should. Could you `assert(!isPreISelGenericOpcode() && !isTargetSpecificOpcode())` inside this if-statement?
> > Sure! That's just what I needed to know: how to describe in a generic way all the instructions like `COPY` here, huge thanks!
> I believe this part should go in separately.
> This is not related to the TableGen change.
It is related in a sense that the TableGen change would break GlobalISel for some of the targets w/o this change.


================
Comment at: lib/CodeGen/GlobalISel/Utils.cpp:62
+           "with no register class constraints");
+    return Reg;
+  }
----------------
qcolombet wrote:
> I believe bailing out won't be enough generally speaking, unless we expect the users of this method to do the right thing for PHIs and COPY.
> 
> E.g., by bailing out on COPYs we could end up with unconstrained VRegs in cases like this:
> v1 = COPY // v1 won't get constrained
> v2 = COPY v1 // <-- v2 will be constrained by its users
> 
> Usually that means we have useless COPYs though.
True, but this patch doesn't make this problem either better or worse. Maybe solve it in a different patch, but commit this one so we don't block ARM folks?


================
Comment at: test/CodeGen/ARM/GlobalISel/arm-select-fptosi-pr35965.mir:2
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple armv7-gnueabihf -run-pass instruction-select -verify-machineinstrs -o - %s | FileCheck %s
+---
----------------
qcolombet wrote:
> Could you use something more descriptive than prxxx in the file name or just drop it?
> 
> Reference the PR in the comment here though.
Sure! I looked around, saw names like this, and thought it's a standard practice to follow =) Glad to know that it's not.


https://reviews.llvm.org/D42565