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

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:132
+  GIM_CheckMemorySizeSmallerThanLLT,
+  GIM_CheckMemorySizeLargerThanLLT,
 
----------------
dsanders wrote:
> rtereshin wrote:
> > 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.
> > 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?
> 
> I avoided LT/GT because GIM_CheckMemorySizeLTLLT/GIM_CheckMemorySizeGTLLT would be rather confusing :-). GIM_CheckMemorySizeLessThanLLT/GIM_CheckMemorySizeGreaterThanLLT makes sense though.
> 
> > Also, looks like "Smaller Than" actually performs <= rather than <, same true for "Larger Than" (>=
> > instead of >), so that makes the names even more confusing.
> 
> They're inverted in the implementation because the code is checking for when to reject it rather than when to accept it.
`LTLLT` is priceless :D

> They're inverted in the implementation because the code is checking for when to reject it rather than when to accept it.

Oh, my bad, didn't notice that.

Thanks for addressing this!


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:291
+      int64_t MMOIdx = MatchTable[CurrentIdx++];
+      uint64_t Size = MatchTable[CurrentIdx++];
+
----------------
dsanders wrote:
> rtereshin wrote:
> > Do we anticipate `MMOIdx` being not `0` here in practice soon? It could be an early generalization, not sure if it needs to be.
> MMO's are stored in a vector in MachineInstr. As far as I know we don't have any cases where there's more than one but I'd prefer to match the representation so we don't have to hunt down all the cases where we made assumptions later.
This is a good argument, yes, but InstructionSelect's input is not arbitrary MachineInstr'uctions, but rather generic instructions, and it looks like there are few of them, they are target-independent, rarely change, and rather restricted.

I agree this is not for this patch (probably?), but let's just keep an eye on it then.

UPD. Ah, I see pseudos mentioned later, that would be the main argument to keep this generic I'd say. Makes sense then!


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


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:320
+      int64_t MMOIdx = MatchTable[CurrentIdx++];
+      int64_t OpIdx = MatchTable[CurrentIdx++];
+
----------------
dsanders wrote:
> rtereshin wrote:
> > ditto + the same question about `OpIdx` and the entire `GIM_CheckMemorySizeLargerThanLLT` opcode.
> > the same question about OpIdx
> 
> We have no guarantees on the value of OpIdx. For G_LOAD/G_STORE and the generic atomics it will be 0 just because that's the relevant operand but targets can define pseudos that do something different.
> 
> > and the entire GIM_CheckMemorySizeLargerThanLLT opcode.
> 
> This one is mostly included for completeness at the moment. However, some targets have stores that do type conversion or vector packing/unpacking as part of the load/store and as a result write more memory than they had bits in the register.
Got it, thanks!


================
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
+
----------------
dsanders wrote:
> rtereshin wrote:
> > `-global-isel` is redundant
> I'm surprised to find that's the case since addGlobalInstructionSelect() is conditional on having either -global-isel or TargetOptions::EnableGlobalISel. Looking at it again, I see that -run-pass builds a custom pipeline and skips that code entirely
> 
> We have -global-isel options everywhere in these tests. Let's clean that up in a separate patch.
Yes, `-run-pass` just sticks whatever required in a custom pipeline, which makes sense.

`-{start,stop}-{before,after}` alter a pipeline built by standard means, of course, so that behaves differently.

I agree on separate patch, thanks!


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:180
+// Track all types that are used so we can emit the corresponding enum.
+std::set<LLTCodeGen> KnownTypes;
+
----------------
dsanders wrote:
> rtereshin wrote:
> > Maybe keeping this a member of `GlobalISelEmitter` is a little better, or rather more consistent with the rest of of implementation.
> I agree it's more consistent but moving it there is going to cause a lot of source churn for little practical benefit. LLTOperandMatcher would need to be able to see GlobalISelEmitter's declaration causing a dependency loop that would require us to separate the declarations and implementations for many of the classes in this file. I don't think it's worth doing that to change a global variable into a static class member.
Maybe we need to get rid of `GlobalISelEmitter` entirely then. 


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1540
+public:
+  enum RelationshipKind {
+    LargerThan,
----------------
dsanders wrote:
> rtereshin wrote:
> > 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.
> Both words have the same definition. I've switched it to Relation on the basis that it's shorter
Thanks!


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1574
+                                          ? "GIM_CheckMemorySizeLargerThanLLT"
+                                          : "GIM_CheckMemorySizeSmallerThanLLT")
+          << MatchTable::Comment("MI") << MatchTable::IntValue(InsnVarID)
----------------
dsanders wrote:
> rtereshin wrote:
> > 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.
> Sharing code between TableGen and CodeGen is tricky. The libraries they have in common are MC and Support.
> 
> These enums are private to CodeGen so it seems wrong to make them part of the public interface for either MC and Support. That said, we've historically put a couple CodeGen things in Support (e.g. LLT and MVT) at least partially to enable sharing with TableGen. In both cases, there would have been substantial code duplication without it.
> 
> I'm leaning towards accepting the duplication until the case for sharing gets stronger. Do you agree?
Yes, let's keep them as is.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1548
+    EqualTo,
+    SmallerThan,
+  };
----------------
Maybe  `LessThan` and `GreaterThan` here too? For consistency as well ;-)


Repository:
  rL LLVM

https://reviews.llvm.org/D45541