This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][NFC] Sorting out Pseudo related classes to avoid confusion
ClosedPublic

Authored by jsji on Nov 30 2018, 1:09 PM.

Details

Summary

There are several Pseudo in PowerPC backend.
eg:

  1. ISel Pseudo-instructions , which has let usesCustomInserter=1 in td ExpandISelPseudos -> EmitInstrWithCustomInserter will deal with them.
  1. Post-RA pseudo instruction, which has let isPseudo = 1 in td, or Standard pseudo (SUBREG_TO_REG,COPY etc.) ExpandPostRAPseudos -> expandPostRAPseudo will expand them
  1. Multi-instruction pseudo operations will expand them PPCAsmPrinter::EmitInstruction
  1. Pseudo instruction in CodeEmitter, which has encoding of 0.

Currently, in td files, especially PPCInstrVSX.td,
we did not distinguish Post-RA pseudo instruction and Pseudo instruction in CodeEmitter very clearly.

This patch is to

  1. Rename Pseudo<> class to PPCEmitTimePseudo, which means encoding of 0 in CodeEmitter
  2. Introduce new class PPCPostRAExpPseudo <> for previous PostRA Pseudo
  3. Introduce new class PPCCustomInserterPseudo <> for previous Isel Pseudo

Diff Detail

Event Timeline

jsji created this revision.Nov 30 2018, 1:09 PM
steven.zhang added inline comments.Dec 3 2018, 7:56 PM
llvm/lib/Target/PowerPC/PPCInstrFormats.td
2167

The logic here is quite weird. Maybe, we could set the isPseudo inside the Pseudo and use the Pseudo as the base class instead of creating a new one. For those places that didn't want the isPseudo to be set, just add a let to clear it. (i.e. when usesCustomInserter is used )

steven.zhang added inline comments.Dec 3 2018, 8:01 PM
llvm/lib/Target/PowerPC/PPCInstrFormats.td
2167

It doesn't make sense to me to not to set the isPseudo for the class Pseudo.

//===----------------------------------------------------------------------===//
class Pseudo<dag OOL, dag IOL, string asmstr, list<dag> pattern>
    : I<0, OOL, IOL, asmstr, NoItinerary> {
  let isCodeGenOnly = 1;
  let PPC64 = 0;
  let Pattern = pattern;
  let Inst{31-0} = 0;
  let hasNoSchedulingInfo = 1;
}
jsji marked an inline comment as done.Dec 3 2018, 8:32 PM
jsji added inline comments.
llvm/lib/Target/PowerPC/PPCInstrFormats.td
2167

As I mentioned in description, there are several "Pseudo", isPseudo flag is only for Post RA Pseudo. While Pseudo<>class are for Code Emitter, so setting isPseudo on Pseudo will causing issues. I don't see any advantage of setting it in base class, then unset it...

Adding PostRAPseudo class is to indicate the difference of Pseudo and PostRAPseudo.
one more comments about "isPseudo is set for PoseRA Pseudo only" may help?

steven.zhang added inline comments.Dec 5 2018, 6:21 PM
llvm/lib/Target/PowerPC/PPCInstrFormats.td
2167

I agree with the intention of the fix. But has some concern on the name PostRAPseudo, which is just set the isPseudo. This is the definition of this flag in Target.td

bit isPseudo     = 0;     // Is this instruction a pseudo-instruction?
                           // If so, won’t have encoding information for
                           // the [MC]CodeEmitter stuff.

It is not relative with PreRA or PostRA. And yes, we use this flag to determine if expand the Pseudo instruction or not. However, it is also used by other place to fold imm etc.

Maybe, we need to distinguish the Pseudo as having the encoding information in CodeEmitter or not. Such as, PsuedoWithoutEncodingInfo, instead of PostRAPseudo.

Unfortunately, the use of this flag varies among platforms. ARM only set it in the base for Pseudo(as what I suggested before), while AArch64 didn't set it in the base, but set it when define the instructions(what we did now).

jsji updated this revision to Diff 177024.Dec 6 2018, 12:14 PM
jsji edited the summary of this revision. (Show Details)

How about this new proposal: Rename base class to PhonyInst to avoid confusion about isPseudo flag.

jsji retitled this revision from [PowerPC][NFC] Set isPseudo in base class PostRAPseudo to [PowerPC][NFC] Sorting out Pseudo related classes to avoid confusion.Dec 6 2018, 12:14 PM
This revision is now accepted and ready to land.Dec 6 2018, 8:50 PM

I am definitely on board with this refactoring since it makes the purpose of the various pseudo instructions more clear.
However, I really don't like the name PhonyInst. These will lead to real instructions being emitted during emit time. I think we should name these according to when we expect to expand these (similarly to what you did for the custom inserter ones).
The names should reflect the uses:

  • Custom inserter
  • Post-RA expansion
  • Emit-time expansion
  • Asm parser expansion (PPCAsmPseudo)

As such, I think it would be nice for them to use a common naming format and to be defined adjacent to one another in the same file. Perhaps something like:

PPCCustomInserterPseudo
PPCPostRAExpPseudo
PPCEmitTimePseudo
PPCAsmPseudo

And in a subsequent orthogonal patch, we may want to consider target flags to indicate the first three of these so that we can add asserts in the code ensuring some of these did not survive past the point where they're supposed to survive (i.e. we shouldn't have any PPCPostRAExpPseudo instructions at emit time).

jsji updated this revision to Diff 177227.Dec 7 2018, 8:36 AM
jsji edited the summary of this revision. (Show Details)

Updated according to Nemanja's comments.

jsji added a comment.Dec 7 2018, 8:36 AM

@nemanjai Would you please have a quick look again? Thanks.

jsji updated this revision to Diff 178055.Dec 13 2018, 7:02 AM

Rebase to trunk.

This revision was automatically updated to reflect the committed changes.