[PATCH] D43962: [GlobalISel][utils] Adding the init version of Instruction Select Testgen
Roman Tereshin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 25 17:26:10 PDT 2018
rtereshin marked 16 inline comments as done.
rtereshin added inline comments.
Herald added a reviewer: javed.absar.
================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:225
/// - TempRegID - The temporary register ID to add
+ /// - TempRegFlags - The register flags to set
GIR_AddTempRegister,
----------------
dsanders wrote:
> Is this comment accurate? GIR_AddRegister is listed as having 2 operands and I only see 2 in my build
The comments in this section describe an opcode **below** the comments, not above. So this is all about `GIR_AddTempRegister` and it appears to have 3 operands (https://github.com/llvm-mirror/llvm/blob/master/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h#L598-L601)
================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorTestgen.h:26-28
+namespace {
+class LiveInRA;
+} // end anonymous namespace
----------------
dsanders wrote:
> This one probably isn't harmful since InstructionSelectorTestgen.h isn't going to be widely included but we ought to avoid anonymous namespaces in headers since each compilation unit that includes the header will get it's own version of the declaration. It's probably best to put it in the llvm namespace if we can't push it into the cpp entirely.
>
> InstructionSelectorTestgen doesn't seem to have any state so it looks like generateTestCase() could be a static function outside the class
That's a good insight, thanks, I didn't think of that.
`generateTestCase` calls virtual member-functions and may do so more in the future and I'd rather keep it that way and keep it a class member.
I'm moving the helper class (`LiveInRA` or however we would (re)name it) in `InstructionSelectorTestgen`as a member-class instead, I hope that solves the problem well enough.
================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorTestgen.h:32-37
+extern cl::opt<unsigned> TestgenFromRule;
+extern cl::opt<unsigned> TestgenUntilRule;
+extern cl::list<unsigned> TestgenExcludeRules;
+extern cl::list<unsigned> TestgenIncludeOnly;
+extern cl::opt<bool> TestgenSetAllFeatures;
+extern cl::opt<bool> TestgenNoABI;
----------------
dsanders wrote:
> Are all of these really needed in the header? Most seem to only be used from InstructionSelectorTestgen.cpp
Only `TestgenSetAllFeatures` is required to be here as it's used by the instruction selector itself.
I'm not exactly sure why not to list all the options in a header, in a sense it's part of its interface, CLI in this case, but sure, I'll remove all of those that don't have to be here. After all, nothing would force this list to be complete, so maybe it's better if it's explicitly incomplete, and also this is rarely (if ever) done across other parts of LLVM.
================
Comment at: include/llvm/Support/CodeGenCoverage.h:26
public:
+ using covered_iterator = BitVector::const_set_bits_iterator;
+
----------------
dsanders wrote:
> Just a small nit: we should probably have 'const' somewhere in the 'covered_iterator' name.
Sure, good catch! I'm renaming it to `const_covered_iterator`
================
Comment at: lib/CodeGen/GlobalISel/InstructionSelectTestgen.cpp:45-47
+ for (const Function &F : M.functions())
+ if (const MachineFunction *MF = MMI.getMachineFunction(F))
+ MF->verify(this, "Pre-existing function", /*AbortOnErrors*/ true);
----------------
dsanders wrote:
> Could you add a comment indicating that verification is the only thing we do with these functions and why?
Sure, adding the following comment:
```
// Generally it is possible to run the Testgen over a non-empty module,
// practically probably a lit-test, and have it add additional test-cases
// to it. The easiest way to limit the number of verification failures on
// the final output caused by initially present machine functions rather
// than the ones added, is to run the verification on those already
// present functions first.
```
================
Comment at: lib/CodeGen/GlobalISel/InstructionSelectorTestgen.cpp:58
+MachineIRBuilder
+InstructionSelectorTestgen::createEmptyTestCase(StringRef Name, Module &M,
+ MachineModuleInfo &MMI) {
----------------
dsanders wrote:
> If the function already exists somehow then this might not return an empty test case due to the getOrInsertFunction() we should probably fail in that case
True. Let's just `MachineFunction::reset()` it for now if such a thing happens, that would make `llc` update the existing test cases if ran over an already generated test-module. It's a sensible thing to do, I think.
Generally, if not reset, it would actually just add another machine basic block and repeat the instruction sequence derived from the selection rule, so we'll end up with a single machine-function test-case with multiple identical basic blocks testing the same pattern. W/ no bug fixes it will screw up the ABI lowering at the moment though, so it will only work for patterns w/ no input vregs, such as having only constants for operands. And that means it will generate a small test-module, it won't fail or crash, but rather just filter out the broken machine functions using machine verifier, as it always does. Or tries to, at least.
Technically, I'm still considering turning this into a sort of a benchmark-gen, not sure at the moment if that could be actually useful though. Theoretically, we could have a tool that will be able to generate long def-use chains of instruction sequences corresponding to a particular pattern or a subset of pattern in a type-driven manner and use them to benchmark the selector as well as other parts of the pipeline.
================
Comment at: lib/CodeGen/GlobalISel/InstructionSelectorTestgen.cpp:102
+ return PointerType::get(Type::getInt32Ty(Context), LLTy.getAddressSpace());
+ return Type::getIntNTy(Context, LLTy.getSizeInBits());
+}
----------------
dsanders wrote:
> LLT's scalars aren't always integers. So long as we inject bitcasts this will be fine though
They aren't, but it appears to me that this is the best we could do w/o going to great lengths, as the information about the actual type is pretty much lost at match table level.
At some point, I tried to lower ABI, specifically to insert an appropriate return sequence, by re-using `CallLoweringInfo::lowerReturn` provided by the target. The method expects an LLVM `Value`, however, the targets only analyze (or were at the moment) the `Value`'s type, so it seemed sufficient to provide it an IR constant of an appropriate IR type.
It didn't work our for a number of reasons, most notably not all the targets could handle all the types, especially vector types. So I started to use the `PATCHABLE_RET` instead for this purpose.
So currently `deduceIRType` is only used to build appropriate machine memory operands to make `GIM_CheckAtomicOrdering` happy, specifically, figure out a reasonable size and alignment. As the size is directly inferable from LLT, this is mostly to figure out the alignment.
================
Comment at: lib/CodeGen/GlobalISel/InstructionSelectorTestgen.cpp:123
+namespace {
+class LiveInRA {
+public:
----------------
dsanders wrote:
> If I understand correctly, this is a register allocator that is used to generate the live-ins for the test function. This needs some explaining in the comments and possibly also renaming (I don't have a good spelling for it, maybe InputRegAllocator)
Your understanding is correct.
I'm adding the following comment:
```
+/// A helper providing sensible phys regs to define patterns' input vregs.
+///
+/// It appears that the most portable and robust way to define all the input
+/// vregs (used, but not defined) of an instruction sequence being generated is
+/// to define them as COPY's from appropriate and preferably distinct physical
+/// registers, live-in to the basic containing the instruction sequence and
+/// preferably the entire function as well. The best pick is to have the same
+/// size in bits as the vreg, same register bank, to be allocatable, and from
+/// the beginning of the allocation order. The class also handles the following
+/// issues:
+///
+/// 1) RegisterBankInfo::getRegBankFromRegClass not being defined for
+/// tablegen'erated reg classes.
+/// 2) Register classes containing the same physical register and yet having
+/// different sizes, and therefore physical registers having only weekly
+/// defined size as the maximum of the sizes of all register classes they
+/// belong to.
+/// 3) Register banks not having a full list of register sizes available
+/// directly.
+///
+/// The latter is also used for picking sensible register banks for internal
+/// (defined and used both by the instruction sequence being generated) vregs.
```
and renaming the class from `LiveInRA` to `InputRegAllocator` (from `(anonymous namespace)::LiveInRA` to `llvm::InstructionSelectorTestgen::InputRegAllocator` to be exact).
================
Comment at: lib/CodeGen/GlobalISel/InstructionSelectorTestgen.cpp:273-308
+ auto selectRegBank = [&MF, &Sizes, &Size2RegBanks, &Hist](unsigned Reg) {
+ MachineRegisterInfo &MRI = MF.getRegInfo();
+ if (MRI.getRegBankOrNull(Reg))
+ return;
+ const LLT LLTy = MRI.getType(Reg);
+ unsigned Size = LLTy.getSizeInBits();
+ const auto SizeI = std::lower_bound(Sizes.begin(), Sizes.end(), Size);
----------------
dsanders wrote:
> This lambda is getting pretty big. I'd be inclined to make it a static function in its own right
Agreed, doing that.
I'm also changing `Size2RegBanksTy` from `DenseMap` to `std::map`, that gets rid of the extracting keys (`Sizes`) and sorting them every time, makes the implementation a little simpler and reduces the number of arguments (former closure captured variables) of the static function being extracted.
================
Comment at: lib/CodeGen/GlobalISel/InstructionSelectorTestgen.cpp:282
+ dbgs() << "- Didn't find a register bank containing allocatable\n"
+ " registers of the required by LLT " << LLTy << " size of\n"
+ " " << Size << " bits or larger for an unconstrained vreg %"
----------------
dsanders wrote:
> I think a word is missing from "of the required by"
An example sentence would be "Didn't find a register bank containing allocatable registers of the required by LLT <2 x s16> size of 32 bits or larger for an unconstrained vreg %1".
Breaking it down as follows:
"Didn't find a register bank containing allocatable registers of a compatible size; vreg: %1(<2 x s16>), size: 32 bits (or larger)."
================
Comment at: lib/CodeGen/GlobalISel/InstructionSelectorTestgen.cpp:306
+ // out of all banks, but only out of a subset of them, update the stats so
+ // we could greadily select the locally best result for the next vreg:
+ ++Hist[RB];
----------------
dsanders wrote:
> greadily -> greedily
Oops, good catch, thanks!
================
Comment at: lib/CodeGen/GlobalISel/InstructionSelectorTestgen.cpp:353
+ << TargetRegisterInfo::virtReg2Index(Reg) << "\n";
+ MIRBuilder.buildInstr(TargetOpcode::IMPLICIT_DEF).addDef(Reg);
+ } else if (TestgenNoABI)
----------------
dsanders wrote:
> I'm not sure IMPLICIT_DEF is the right thing to use for these if-statements. IMPLICIT_DEF is a definition with an unknown value (much like UNDEF) so it wouldn't be wrong to propagate it in something like
> %0 = IMPLICIT_DEF
> %2 = G_ADD %0, %1
> to:
> %2 = IMPLICIT_DEF
> a COPY of a live-in phys-reg would be safer but it's probably ok since constant propagation isn't ISel's job. I think this would only become an issue if we started porting this to combiners.
> COPY of a live-in phys-reg would be safer
It is, this is why I'm using `IMPLICIT_DEF` only if I couldn't find a phys-reg with the appropriate size and within the required register bank (or if this behavior is explicitly requested by a command line option).
================
Comment at: lib/CodeGen/GlobalISel/InstructionSelectorTestgen.cpp:366
+ uint64_t CurrentIdx) {
+ static constexpr unsigned NumberOfOperands[GIU_NumberOfOpcodes] = {
+ 1 /*GIM_Try*/,
----------------
dsanders wrote:
> This table is going to be quite fragile. We should at put this somewhere near the GIM_*/GIR_* declarations or at least cross-reference them in the comments. Tablegen-erating them might be sensible if we get additional metadata.
Agreed. I'm making this less fragile by doing the following:
# Specifying a list of pairs implicitly defining a mapping from an opcode to its number of operands instead so
# there is no need to maintain the records in a specific order, matching the order of the opcode definitions
# opcodes are used directly, not just as a comment, thus making sure there are no non-existent opcodes mentioned in the list
# adding assertions that would make sure that there are no opcodes missing from the mapping
# putting the definition right next to the opcode definitions
================
Comment at: lib/CodeGen/GlobalISel/InstructionSelectorTestgen.cpp:468
+
+ if (InsnB->getNumOperands() == OpIdx)
+ OperandAdder(InsnB);
----------------
dsanders wrote:
> Is this ever false? ensureNumOperands() looks like it adds operands until this is true
Of course, the number of operands could be greater than `OpIdx`, `ensureNumOperands` doesn't add or remove operands in that case.
================
Comment at: lib/CodeGen/GlobalISel/InstructionSelectorTestgen.cpp:484
+
+static const LLT DummyLLT = LLT::scalar(32);
+// Assuming that all the parts of the MatchTable that don't affect the selection
----------------
dsanders wrote:
> What is this for?
Often I need to create a generic virtual register w/o knowing which type is expected from it yet, and it's not exactly possible to create a virtual register w/o a type. This constant exists to be consistent with the type I use as a default / initial option.
================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:3983-3995
+ OS << "bool " << Target.getName()
+ << "InstructionSelector::selectImpl(MachineInstr &I, CodeGenCoverage "
+ << "&CoverageInfo) const {\n"
+ << " MachineFunction &MF = *I.getParent()->getParent();\n"
+ << " MachineRegisterInfo &MRI = MF.getRegInfo();\n"
+ << " // FIXME: This should be computed on a per-function basis rather "
+ << "than per-insn.\n"
----------------
dsanders wrote:
> It seems that this block has been moved down from the other side of the sort. Non-functional changes like this ought to be in separate patches
I'm extracting a number of patches from this one to break it down a little.
Repository:
rL LLVM
https://reviews.llvm.org/D43962
More information about the llvm-commits
mailing list