[PATCH] D72034: [GlobalISel][RFC] Importing patterns with PtrValueType and nullptr

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

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)



================
Comment at: llvm/lib/Target/Mips/MipsGISel.td:1
+include "Mips.td"
+
----------------
The indentation in this file is all over the place


================
Comment at: llvm/test/CodeGen/Mips/GlobalISel/instruction-select/icmp.mir:19
+  define void @uge_ptr() {entry: ret void}
+  define void @ult_ptr() {entry: ret void}
+  define void @ule_ptr() {entry: ret void}
----------------
I don't think you need the IR section in this test at all


================
Comment at: llvm/test/CodeGen/Mips/GlobalISel/instruction-select/select_patterns.mir:3
+# RUN: llc -O0 -mtriple=mipsel-linux-gnu -run-pass=instruction-select -verify-machineinstrs %s -o - | FileCheck %s -check-prefixes=MIPS32FP32
+--- |
+  define i32* @select(i32* %Test, i32* %T, i32* %F) {
----------------
Shouldn't need the IR section


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72034/new/

https://reviews.llvm.org/D72034