Page MenuHomePhabricator

[GlobalISel][RFC] Importing patterns with PtrValueType and nullptr
Needs ReviewPublic

Authored by Petar.Avramovic on Dec 31 2019, 3:59 AM.

Details

Summary

This is WIP on iseling icmp, select and branch patterns that involve pointers.

Pointers in GlobalISel have pointer type, unlike SDAG where they have integer type.
Thus GlobalISel selectImpl cannot select pointer instructions from SDAG td files since there are no patterns for pointers there.

It is already possible to write td file for GlobalIsel that will allow selection of pointer patterns.
For that reason I am adding a new td file with additional patterns for pointers, everything from SDAG td files is also included.

Introducing new ValueType, NullPtrValue. It allows to specify nullpointer integer value in td file that will be matched with:

%0:gprb(scalar) = G_CONSTANT intvalue
%1:gprb(p_addrspace) = G_INTTOPTR %0(scalar)

since that is how globalisel translates llvm-ir null. For MIPS intvalue is 0.

Added a few pointer patterns example tests for MIPS in new td file.

Similar thing could be possible for x86, this pattern for example:

def : Pat<(X86cmp GR32:$src1, 0),
          (TEST32rr GR32:$src1, GR32:$src1)>;

also works for pointers in SDAG, but for globalisel it would require custom legalize that generates GINodeEquiv of X86cmp and new pattern with PtrValueType and NullPtrValue instead of 0.

TODO: If NullPtrValue idea works, split to smaller patches and add detailed tests for source patterns with two or more instructions.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptDec 31 2019, 3:59 AM

G_CONSTANT can already directly have a pointer type, so I don't see a reason that the extra G_INTTOPTR needs to be involved. I would also like to avoid that for non-integral address spaces, which may still have a sentinel pointer value / null value. This is also going to create a lot of spurious instructions. It's not just the selection patterns that will need to consider this, but any pattern matching before that.

Are you trying to specifically select pointer differently from integers? I thought you could already get this to work without distinguishing them with PtrValueType. My main frustration with pointers today in legalization and selection patterns is treating most address spaces as "other". i.e. I have a whitelist of address spaces to handle specially, but anything else should be treated as if it were address space 1. This seems to commit to defining more address space specific machinery (although I suppose I can fixup all of these cases in legalization)

llvm/lib/Target/Mips/MipsGISel.td
1

The indentation in this file is all over the place

llvm/test/CodeGen/Mips/GlobalISel/instruction-select/icmp.mir
19

I don't think you need the IR section in this test at all

llvm/test/CodeGen/Mips/GlobalISel/instruction-select/select_patterns.mir
3

Shouldn't need the IR section

Petar.Avramovic marked an inline comment as done.Jan 3 2020, 1:26 AM

G_CONSTANT can already directly have a pointer type, so I don't see a reason that the extra G_INTTOPTR needs to be involved.

When I started this patch I thought the same. nullptr was translated like that some time ago, D44762 changed it to what we have now. NullPtrValue was meant to match IRTranslator's translation of llvm-ir null

Are you trying to specifically select pointer differently from integers?

I am trying to select pointers/nullptr exectly like integers i.e. use gpr32/0.
Sdag td files already have Int_Src -> Int_Dst patterns.
I am trying to add Pointer_Src -> Int_Dst that will be used by GlobalISel only since it has pointers in intermediate representation.
It looks like this:

def : MipsPat<(seteq GPR32:$lhs, GPR32:$rhs),
              (SLTiu (XOR GPR32:$lhs, GPR32:$rhs), 1)>;

def : MipsPat<(seteq p0:$lhs, p0:$rhs),
              (SLTiu (XOR GPR32:$lhs, GPR32:$rhs), 1)>;

There should be fewer pointer patterns then integer patterns.

I thought you could already get this to work without distinguishing them with PtrValueType.

If you think of changing pointer type to scalar before isel, I also think that could work (it works in sdag).
I asked about it in http://lists.llvm.org/pipermail/llvm-dev/2019-November/137041.html.

llvm/test/CodeGen/Mips/GlobalISel/instruction-select/icmp.mir
19

I see. I will take a look at other mir tests and fix them all together.

Ping.

I'm going to have to disagree with the change in D44762, and think it should be reverted. null in the IR has the original pointer type, and is not a cast from an integer constant. This presents a problem for non integral address spaces for example, where you are allowed to have a literal null/0 value, but the inttoptr is not allowed. I would prefer if the IR defined a more proper nullptr in the IR that wasn't just a 0 constant, but that's not the current state of the world.