[PATCH] D45541: [globalisel] Update GlobalISel emitter to match new representation of extending loads

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added a comment.
Herald added a reviewer: javed.absar.

Hi Daniel,

My 2 cents, mostly nitpicks. This is not a full review, just some stuff that caught my eye. Thanks!



================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:132
+  GIM_CheckMemorySizeSmallerThanLLT,
+  GIM_CheckMemorySizeLargerThanLLT,
 
----------------
It appears to me that quite universally orderings are called "less than" and "greater than" with well known LT and GT abbreviations, maybe it makes sense to stick to the widely used naming scheme?

Also, looks like "Smaller Than" actually performs `<=` rather than `<`, same true for "Larger Than" (`>=` instead of `>`), so that makes the names even more confusing.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:291
+      int64_t MMOIdx = MatchTable[CurrentIdx++];
+      uint64_t Size = MatchTable[CurrentIdx++];
+
----------------
Do we anticipate `MMOIdx` being not `0` here in practice soon? It could be an early generalization, not sure if it needs to be.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:302
+        if (handleReject() == RejectAndGiveUp)
+          return false;
+
----------------
Otherwise it needs to break out of the case, or the statement below will cause an out of bounds access.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:320
+      int64_t MMOIdx = MatchTable[CurrentIdx++];
+      int64_t OpIdx = MatchTable[CurrentIdx++];
+
----------------
ditto + the same question about `OpIdx` and the entire `GIM_CheckMemorySizeLargerThanLLT` opcode.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:345
+        if (handleReject() == RejectAndGiveUp)
+          return false;
+
----------------
ditto


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-sextload.mir:2
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=aarch64-- -run-pass=instruction-select -verify-machineinstrs -global-isel %s -o - | FileCheck %s
+
----------------
`-global-isel` is redundant


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-sextload.mir:18
+  bb.0:
+    liveins: $w0
+
----------------
`$x0`


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-sextload.mir:36
+  bb.0:
+    liveins: $w0
+
----------------
`$x0`


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-zextload.mir:2
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=aarch64-- -run-pass=instruction-select -verify-machineinstrs -global-isel %s -o - | FileCheck %s
+
----------------
`-global-isel` command line option is redundant here.


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-zextload.mir:24
+    ; CHECK: $w0 = COPY [[T0]]
+    %0:gpr(p0) = COPY $x0
+    %1:gpr(s32) = G_ZEXTLOAD %0 :: (load 2 from %ir.addr)
----------------
live in is `w0`, but `x0` is used, this is a border-line valid MIR.


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-zextload.mir:42
+    ; CHECK: $w0 = COPY [[T1]]
+    %0:gpr(p0) = COPY $x0
+    %1:gpr(s16) = G_LOAD %0 :: (load 2 from %ir.addr)
----------------
ditto


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:180
+// Track all types that are used so we can emit the corresponding enum.
+std::set<LLTCodeGen> KnownTypes;
+
----------------
Maybe keeping this a member of `GlobalISelEmitter` is a little better, or rather more consistent with the rest of of implementation.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1517
+  MemorySizePredicateMatcher(unsigned InsnVarID, unsigned MMOIdx, unsigned Size)
+      : InstructionPredicateMatcher(IPM_MemoryLLTSize, InsnVarID),
+        MMOIdx(MMOIdx), Size(Size) {}
----------------
This class (`MemorySizePredicateMatcher`) having the exact same kind (`IPM_MemoryLLTSize`) as `MemoryVsLLTSizePredicateMatcher` doesn't seem right.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1525
+    return InstructionPredicateMatcher::isIdentical(B) &&
+           Size == cast<MemorySizePredicateMatcher>(&B)->Size;
+  }
----------------
It feels like we lost `MMOIdx` here.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1540
+public:
+  enum RelationshipKind {
+    LargerThan,
----------------
I believe the math term for this sort of things is "relation", not "relationship" (it's also shorter). I've also seen name "ordering" used for such enums, like in Haskell. "Relationship" feels a little odd, or maybe just unnecessary long.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1565
+               cast<MemoryVsLLTSizePredicateMatcher>(&B)->Relationship &&
+           OpIdx == cast<MemoryVsLLTSizePredicateMatcher>(&B)->OpIdx;
+  }
----------------
It feels like we lost `MMOIdx` here.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1574
+                                          ? "GIM_CheckMemorySizeLargerThanLLT"
+                                          : "GIM_CheckMemorySizeSmallerThanLLT")
+          << MatchTable::Comment("MI") << MatchTable::IntValue(InsnVarID)
----------------
The same is done to produce the debugging output in `executeMatchTable` and in a slightly different style. Maybe it makes sense to extract this as a separate `Relationship -> StringRef` function.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:3506
+                                  llvm::to_string(*P.getDstPattern()) << "\n";
+  errs() << "Rule ID " + llvm::to_string(M.getRuleID()) << "\n";
 
----------------
Maybe wrap this in a `DEBUG` (or a friend) or eliminate entirely. Also, do we really need `llvm::to_string` here, just streaming it with `<<` doesn't work? Especially for the RuleID, it's just a number IIRC.


Repository:
  rL LLVM

https://reviews.llvm.org/D45541