This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][GIsel] Materialize i64 constants.
ClosedPublic

Authored by Kai on Dec 15 2022, 10:18 AM.

Details

Summary

Adds support for i64 constant. It uses the same pattern-based
approach as in SDAG (see PPCISelDAGToDAG::selectI64ImmDirect(),
PPCISelDAGToDAG::selectI64Imm()). It does not support the
prefixed instructions.

Diff Detail

Event Timeline

Kai created this revision.Dec 15 2022, 10:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2022, 10:18 AM
Kai requested review of this revision.Dec 15 2022, 10:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2022, 10:18 AM
arsenm added inline comments.Dec 15 2022, 10:24 AM
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
360

Stick with uint32_t return type?

362

uint32_t throughout?

543

BuildMI actually has a .constrainAllUses(). Can use that and drop this MI argument used everywhere?

Kai updated this revision to Diff 483270.Dec 15 2022, 11:42 AM
  • Use uint32_t in findContiguousZerosAtLeast()
  • Replace use of constrainSelectedInstRegOperands() with constrainAllUses()
  • Uses std::optional<bool> instead of MI parameter
Kai marked 3 inline comments as done.Dec 15 2022, 11:42 AM
arsenm added inline comments.Dec 15 2022, 11:46 AM
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
707

I don't understand the point of the optional, just make it a bool

Kai added inline comments.Dec 15 2022, 11:55 AM
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
707

The function has 3 outcomes: It returns the result from constrainAllUses() (a bool), but it can also fail to produce a an instruction if none of the pattern the pattern matches.
If I just return a bool, then I have to assume that constrainAllUses() always returns true. Is that ok?

arsenm accepted this revision.Dec 15 2022, 12:05 PM
arsenm added inline comments.
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
692–693

You can just unconditionally erase, if it failed the whole function's getting deleted anyway

This revision is now accepted and ready to land.Dec 15 2022, 12:05 PM
tschuett added inline comments.
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
573

negative

689

are

This revision was landed with ongoing or failed builds.Dec 15 2022, 1:23 PM
This revision was automatically updated to reflect the committed changes.

hmm, so many duplicated codes, but seems there is no simple way to avoid this.

D139813 adds some improvement for i64 materialization in DAG ISel. That should be adopted to GISel too.

This reminds me there was a proposal (https://reviews.llvm.org/D82709#2137837) for PPC target that PPC can materialize the i64 imm after ISEL and in ISEL just lower the i64 imm to a pseudo instruction. So we can avoid the duplication here and also some other remat issue in register allocation. But that may require big effort...

Kai added a comment.Dec 16 2022, 8:33 AM

hmm, so many duplicated codes, but seems there is no simple way to avoid this.

D139813 adds some improvement for i64 materialization in DAG ISel. That should be adopted to GISel too.

This reminds me there was a proposal (https://reviews.llvm.org/D82709#2137837) for PPC target that PPC can materialize the i64 imm after ISEL and in ISEL just lower the i64 imm to a pseudo instruction. So we can avoid the duplication here and also some other remat issue in register allocation. But that may require big effort...

I agree that the code duplication is unfortune. There is also the comment in PPCInstrInfo::materializeImmPostRA():

// FIXME: Materialization here is not optimal.
// For some special bit patterns we can use less instructions.
// See `selectI64ImmDirect` in PPCISelDAGToDAG.cpp.

Well, we would need:

  • a new pseudo instruction loadimmi64
  • a pass to expand the pseudo after ISEL
  • in best case the implementation is shareable with materializeImmPostRA()

Sounds doable.