This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/GlobalISelemitter Support for predicate code that uses operands
ClosedPublic

Authored by Petar.Avramovic on Sep 8 2020, 6:33 AM.

Details

Summary

Predicates with 'let PredicateCodeUsesOperands = 1' want to examine
matched operands. When we encounter predicate code that uses operands,
analyze its named operand arguments and create a map between argument
index and name. Later, when leaf node with name is encountered,
emit GIM_RecordNamedOperand that will store that operand at its
argument index in a operand list. This operand list will be argument
to c++ code of the predicate.

Diff Detail

Event Timeline

Petar.Avramovic created this revision.Sep 8 2020, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2020, 6:33 AM
Petar.Avramovic requested review of this revision.Sep 8 2020, 6:33 AM
arsenm added inline comments.Sep 8 2020, 7:15 AM
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
257

Can you be clearer on what "uses operands" means here?

457

Is this the recorded use or def operand? Can you add a comment here?

llvm/lib/CodeGen/GlobalISel/InstructionSelector.cpp
33 ↗(On Diff #290469)

Why arbitrarily resize this here?

llvm/lib/Target/AMDGPU/VOP3Instructions.td
607–609

Should update this comment

611–612

As a follow up, we need to do something about inline constants which will end up blocking this. I was thinking we need to start producing constants with VGPR bank with VALU uses prior to selection

llvm/test/CodeGen/AMDGPU/GlobalISel/add_shl.ll
3–5

Can you move the < %s to the end

llvm/utils/TableGen/GlobalISelEmitter.cpp
4208

Can you move the decrement out of the if?

Petar.Avramovic added inline comments.Sep 8 2020, 7:46 AM
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
457

As far as I know operands in this list are taken from instruction where they were use operands. Was that the question?

llvm/lib/CodeGen/GlobalISel/InstructionSelector.cpp
33 ↗(On Diff #290469)

Operands are inserted directly into specific index in RecordedOperands, see GIM_RecordNamedOperand.
Magic number 3 is the largest number of operands that any of the patterns with 'let PredicateCodeUsesOperands = 1' has.
I wanted to avoid having to keep additional maps between operand names and indices in final operand list that will be sent as argument to c++ predicate code' (like sdag). Unlike sdag, globalisel emitter first visits the predicate and we can figure out indices right away, sdag visits all operands first and predicate is visited last.

foad added inline comments.Sep 8 2020, 8:55 AM
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h
525

Can't this be an ArrayRef<MachineOperand *> ? (Or a pointer to const std::array ?)

llvm/lib/CodeGen/GlobalISel/InstructionSelector.cpp
33 ↗(On Diff #290469)

If you really want a fixed size array, wouldn't std::array be better?

Add more verbose comments, and switch to std::array as container for operands.

arsenm added inline comments.Sep 9 2020, 7:39 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
3923

Can we put this string generation in a function somewhere? I think this is repeated 2 or 3 times

Factor string generation of operand names into function.

arsenm added inline comments.Sep 10 2020, 4:44 PM
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
631

Should assert StoreIdx < array size

631

Alternatively, tablegen could error if it hits the hardcoded limit?

Add assert for StoreIdx.

arsenm accepted this revision.Sep 11 2020, 2:13 PM

LGTM, but a tablegen error would be more discoverable when somebody hits the limit

This revision is now accepted and ready to land.Sep 11 2020, 2:13 PM