This change introduces the HasNoUse builtin predicate in PatFrags that
checks for the absence of use of the first result operand.
GlobalISelEmitter will allow source PatFrags with this predicate to be
matched with destination instructions with empty outs. This predicate is
required for selecting the no-return variant of atomic instructions in
AMDGPU.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
60,110 ms | x64 debian > LLVM.CodeGen/NVPTX::wmma.py |
Event Timeline
Would it be helpful if the generated matcher actually checked (or maybe asserted) that the discarded defs are unused?
This makes sense if the we don't have predicates that check for the use of the defs; Like in PowerPC:
def int_ppc_trechkpt : GCCBuiltin<"__builtin_trechkpt">, Intrinsic<[llvm_i32_ty], [], []>; def TRECHKPT : XForm_base_r3xo <31, 1006, (outs), (ins), "trechkpt." ... def : Pat<(int_ppc_trechkpt), (TRECHKPT)>;
I don't think it would be useful for D125213.
I do think there should be a builtin hasOneUse pattern predicate the emitter could use to verify
If we have a bit HasNoUse builtin predicate in PatFrags we can get the matcher
table to check the empty usage of the result without the generic C++ predicates.
The problem I'm seeing here is that the default behaviour should be to not check
for the use count since most of the selection doesn't care about it. Having a
ternary HasNoUse builtin is fine since we could set it to false for selection
that needs at least 1 use (e.g.: AMDGPU's return atomic ops), true for selection
that needs no use (e.g.: AMDGPU's no return atomic ops), "don't care" for the
majority of the other selections. I don't think we can rely on the '?'
(uninitialized) value since TreePredicateFn::isPredefinedPredicateEqualTo()
return false for uninitialized fields.
How should we do this?
Having a false/unset builtin HasOneUse predicate doesn't imply applying the negated predicate. It would just be nothing. If we wanted a hasOneOrMultiple predicate, it would be a separate PatFrag bit
I'm assuming you mean has(No)Use, I don't see the relevance of checking for one
use here.
I agree with you on omitting the predicates if the field is set to
false/uninitialized. In that case, how about we add the following to
PatFrags:
// If set to true, a predicate is added that checks for at least one use of // the return value; Otherwise no predicate is added. bit HasUse = ?; // If set to true, a predicate is added that checks for no use of the return // value; Otherwise no predicate is added. // // The TableGen backend asserts that both HasUse and HasNoUse is not set to // true. bit HasNoUse = ?;
Yes. The way the PatFrags are structured this is two different predicates the emitter needs to enforce are mutually exclusive. This only really makes sense if you look at how this is implemented
llvm/test/TableGen/GlobalISelEmitter-HasUse.td | ||
---|---|---|
45 | A philosophical question: do we really need HasUse? After all the atomic_load_add_ret pattern will still work in both cases (use & nouse), so shouldn't we use priorities instead to prefer the atomic_load_add_no_ret pattern when both are applicable? | |
llvm/utils/TableGen/GlobalISelEmitter.cpp | ||
5289 | Use min() ? | |
5291–5292 | This looks odd. Does the call to getExtType have side effects? |
llvm/test/TableGen/GlobalISelEmitter-HasUse.td | ||
---|---|---|
45 | Yes, having one predicate and priorities would be more consistent | |
llvm/utils/TableGen/GlobalISelEmitter.cpp | ||
4037–4042 | It's very not obvious but the code is structured in such a way that assumes each level of PatFrag you define only defines one predicate. We need verification (like the DAG path has) that this is the case | |
5271 | The naming here is potentially confusing, since it looks like it's checking for a use of the predicate itself. |
llvm/utils/TableGen/GlobalISelEmitter.cpp | ||
---|---|---|
4037–4042 | I'm interested in reworking this, but probably in a different patch. |
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h | ||
---|---|---|
199 | Should document that it applies to the first result operand - if that's what you intended to implement. | |
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h | ||
390 | Don't need == nullptr. | |
llvm/include/llvm/Target/TargetSelectionDAG.td | ||
789 | "... the first result"? |
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h | ||
---|---|---|
390 | Don't we need to make sure that State.MIs[InsnID] is not a nullptr here? |
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h | ||
---|---|---|
390 | I just meant you could write it as assert(MI && "Used insn before defined") - you don't need the != nullptr part. (And I mistyped == instead of !=.) |
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h | ||
---|---|---|
392 | Ideally this would be for a specific operand index, but it would be a bit hard to test since we still can't write patterns with other defs | |
llvm/include/llvm/Target/TargetSelectionDAG.td | ||
787 | Not sure how you can avoid supporting it here? |
llvm/include/llvm/Target/TargetSelectionDAG.td | ||
---|---|---|
787 | Yes. I think not having the predicate work in both would be confusingly divergent. I think the DAG path is easier to handle since it's just dumb C++ emission |
- Rebase
- Add the SelectionDAG implementation; Had to add this in the TreePredicateFn::hasPredCode() since the predicate was not added for intrinsics without it (due to the absence of IsAtomic?).
Should document that it applies to the first result operand - if that's what you intended to implement.