I’m creating this thread with the hopes of sparking a discussion regarding how differences between the DAG/GISel should be addressed when writing TableGen patterns.
I have encountered multiple GlobalISel pattern-matching difficulties in the past few weeks, and the latest occurrence is in D132483.
In essence, the typical scenario is that a pattern that we try to match is straightforward in the DAG, but more intricate/varied in gMIR. Those differences can be very difficult - or even impossible - to cleanly express in TableGen.
This thread is a bit of a continuation of a previous thread started by @jayfoad: globalisel: cross-bank constant propagation? - #5 by jayfoad
I’m a fairly new contributor to GISel, I still have much to learn. Take everything I wrote here with a pinch of salt.
The above diff tries to improve generation of the
V_BFI_B32 instruction in the AMDGPU backend when GlobalISel is being used. Take the following pattern, for example:
def : AMDGPUPat < (DivergentBinFrag<xor> i32:$z, (and i32:$x, (xor i32:$y, i32:$z))), (V_BFI_B32_e64 VSrc_b32:$x, VSrc_b32:$y, VSrc_b32:$z) >;
This simple pattern can sometimes fail to match due to copies between register banks, which are common the the AMDGPU backend (they’re always inserted to transforrm between VGPR/SGPR). Thus, the following gMIR cannot match the pattern above:
%4:sgpr(s32) = G_XOR %1:sgpr, %2:sgpr %7:vgpr(s32) = COPY %4:sgpr(s32) ; COPY prevents pattern matching %5:vgpr(s32) = G_AND %0:vgpr, %7:vgpr %6:vgpr(s32) = G_XOR %2:sgpr, %5:vgpr
As far as I know, there is no way to tell TableGen to ignore COPY instructions currently. Thus, we need to get rid of the copy (which the diff tries to do, but we quickly run into other - more complex - issues), or perform instruction selection in C++ (which is annoying).
A while ago, @jayfoad started a patch to introduce a dummy DAG operator that can be used to tell TableGen that a copy may be present in gMIR. There was a lot of discussion about it but it never landed.
Personally, I don’t think this is the right approach. It seems innocent enough to add a
maybe_copy operation in every affected pattern. It reminds me a bit of using
null in programming language design: it starts small, but it quickly finds its way everywhere. Much like you’d check for
null values every other line in some language, I fear that we’ll find ourselves adding
maybe_copy every other operation in patterns.
Another option would be to consistently ignore cross registerbank copies in GISel when checking opcodes. e.g.
COPY, but the expected opcode is not
COPY, it checks if it’s a cross-registerbank copy. If it is, it looks past it to try and find the correct instruction.
COPYwould only be considered as cross-registerbank if the input/output type is strictly equal, and the only difference between the input/output VReg is the register bank being different.
This would immediately solve the problem, and I think short-term it would have less adverse effects. There’s a few concerns with this but I believe they could be addressed rather easily.
The biggest concern for me is if we want to disable this behavior in some cases, e.g. a pattern shouldn’t match if the operand is a copy. I think we could solve this by adding a special operator (e.g.
direct, for instance) to express that copies are undesired for a given instruction’s operands.
// could match even if there's a COPY between the adds. (add $x, (add $y, $z) // cannot match if there's a COPY between the adds (add $x, (nocopy (add $y, $z))
This has the same drawbacks as
maybe_copy, but I think it would be used less often and thus would be less of an issue.
How do you think we should move forward with those issues? Are any of the proposed solution acceptable at this time? Is there a better solution I’m missing?
Thanks a lot!