This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel][SelectionDAG] Implement the HasNoUse builtin predicate
ClosedPublic

Authored by abinavpp on May 9 2022, 1:56 AM.

Details

Summary

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.

Diff Detail

Event Timeline

abinavpp created this revision.May 9 2022, 1:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 1:56 AM
Herald added a subscriber: rovka. · View Herald Transcript
abinavpp requested review of this revision.May 9 2022, 1:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 1:56 AM
foad added a comment.May 9 2022, 2:10 AM

Would it be helpful if the generated matcher actually checked (or maybe asserted) that the discarded defs are unused?

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.

arsenm added a comment.May 9 2022, 2:49 AM

I do think there should be a builtin hasOneUse pattern predicate the emitter could use to verify

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?

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 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 = ?;

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

abinavpp updated this revision to Diff 430971.May 20 2022, 7:51 AM

Change the SDNPHasChain approach to the Has{No}Use predicates approach

abinavpp retitled this revision from [GlobalISel] Allow destination patterns having empty outs to [GlobalISel] Implement the Has{No}Use builtin predicates.May 20 2022, 7:52 AM
abinavpp edited the summary of this revision. (Show Details)
foad added inline comments.Jun 9 2022, 2:38 AM
llvm/test/TableGen/GlobalISelEmitter-HasUse.td
45 ↗(On Diff #430971)

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
5268–5269

Use min() ?

5270–5271

This looks odd. Does the call to getExtType have side effects?

arsenm added inline comments.Jun 9 2022, 5:39 PM
llvm/test/TableGen/GlobalISelEmitter-HasUse.td
45 ↗(On Diff #430971)

Yes, having one predicate and priorities would be more consistent

llvm/utils/TableGen/GlobalISelEmitter.cpp
4030–4035

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

5258

The naming here is potentially confusing, since it looks like it's checking for a use of the predicate itself.

abinavpp updated this revision to Diff 438553.Jun 20 2022, 9:50 PM

Address review comments

abinavpp added inline comments.Jun 20 2022, 9:52 PM
llvm/test/TableGen/GlobalISelEmitter-HasUse.td
45 ↗(On Diff #430971)
abinavpp marked 3 inline comments as done.Jun 20 2022, 9:54 PM
abinavpp added inline comments.
llvm/utils/TableGen/GlobalISelEmitter.cpp
4030–4035

I'm interested in reworking this, but probably in a different patch.

abinavpp retitled this revision from [GlobalISel] Implement the Has{No}Use builtin predicates to [GlobalISel] Implement the HasNoUse builtin predicate.Jun 20 2022, 9:55 PM
abinavpp edited the summary of this revision. (Show Details)
foad added inline comments.Jun 21 2022, 3:24 AM
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
809

"... the first result"?

abinavpp updated this revision to Diff 438632.Jun 21 2022, 4:16 AM

Address review comments

abinavpp marked 2 inline comments as done.Jun 21 2022, 4:23 AM
abinavpp added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
390

Don't we need to make sure that State.MIs[InsnID] is not a nullptr here?

foad added inline comments.Jun 21 2022, 4:53 AM
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 !=.)

abinavpp updated this revision to Diff 438641.Jun 21 2022, 5:03 AM

Address review comment

abinavpp marked an inline comment as done.Jun 21 2022, 5:04 AM
arsenm added inline comments.Jun 23 2022, 3:45 PM
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
807

Not sure how you can avoid supporting it here?

abinavpp added inline comments.Jun 24 2022, 11:39 AM
llvm/include/llvm/Target/TargetSelectionDAG.td
807

I assume we're good with the changes for GlobalISel (including D128241 and D125213). Should I add the SelectionDAG support in this patch?

arsenm added inline comments.Jun 24 2022, 3:41 PM
llvm/include/llvm/Target/TargetSelectionDAG.td
807

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

abinavpp updated this revision to Diff 442053.Jul 4 2022, 3:04 AM
  • 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?).
abinavpp marked an inline comment as done.Jul 4 2022, 3:06 AM
abinavpp retitled this revision from [GlobalISel] Implement the HasNoUse builtin predicate to [GlobalISel][SelectionDAG] Implement the HasNoUse builtin predicate.
abinavpp edited the summary of this revision. (Show Details)
arsenm added a comment.Jul 5 2022, 9:22 AM
  • 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?).

This shouldn't depend on the memory properties. This should work for any node

arsenm accepted this revision.Jul 5 2022, 9:24 AM
This revision is now accepted and ready to land.Jul 5 2022, 9:24 AM
This revision was landed with ongoing or failed builds.Jul 7 2022, 9:22 PM
This revision was automatically updated to reflect the committed changes.