Page MenuHomePhabricator

[NFC][PowerPC]Clean up PPCAsmPrinter for TOC related pseudo opcode
ClosedPublic

Authored by Xiangling_L on Wed, Oct 9, 12:22 PM.

Diff Detail

Event Timeline

Xiangling_L created this revision.Wed, Oct 9, 12:22 PM
jsji accepted this revision.Wed, Oct 9, 12:54 PM

LGTM. Some nit comments.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
744

MOSymbol used only once here, maybe inline the call directly?

763

large code model only for CPI?

776

Are these clang formatted? Formatting looks weird to me.

799

large code model only for CPI?

837

MOSymbol used only once here, maybe inline the call directly?

This revision is now accepted and ready to land.Wed, Oct 9, 12:54 PM
Xiangling_L marked 6 inline comments as done.Wed, Oct 9, 1:15 PM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
776

Yes, they are.

Xiangling_L marked an inline comment as done.

Address commments & add const for lookUpOrCreateTOCEntry & TOC MapVector<const MCSymbol *, MCSymbol *>

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
517

Has a switch on getType been considered?

687

Is it also possible to use * const?

739

There's an extra space in the indentation here.

762

Please don't remove the Oxford comma before the "or".

773

Can this be const?

776

This does not seem to be an NFC change.

799

Comma before "then".

800

Comma after "Otherwise".

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
838

Missing the invocation of assert here?

1378–1379

Has a range-based for loop been considered?

Xiangling_L marked 13 inline comments as done.Thu, Oct 10, 7:50 AM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
517

Sure.

776

Since Machine Operand for this TOC pseudo opcode can only be MO.isGlobal() || MO.isCPI() || MO.isJTI() || MO.isBlockAddress(), so the line`(MO.isCPI() && TM.getCodeModel() == CodeModel::Large) will have the same logic with or without MO.isCPI(). But since people @jsji have some slight preference to make it clear, I add MO.isCPI()`. Please let me know if you still have any concerns about this change.

838

Thank you for mentioning this mistake.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
776

I do not believe that the statement is sufficiently supported in this context except if there is:

assert(GlobalToc || !MO.isGlobal());
Xiangling_L marked 3 inline comments as done.

Address 2nd round comments

Xiangling_L marked 2 inline comments as done.Thu, Oct 10, 8:35 AM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
776

I think I kinda know your concern, do you mean without my changes, for query if (GlobalToc || MO.isJTI() || MO.isBlockAddress() || TM.getCodeModel() == CodeModel::Large), there is the possibility that GlobalToc is false, but the code model is large, under which we still generate TOCEntry for global MO? If my understanding is correct, then I think we don't need to worry about this situation, because according to the definition of isGVIndirectSymbol:

bool PPCSubtarget::isGVIndirectSymbol(const GlobalValue *GV) const {
  // Large code model always uses the TOC even for local symbols.
  if (TM.getCodeModel() == CodeModel::Large)
    return true;
  if (TM.shouldAssumeDSOLocal(*GV->getParent(), GV))
    return false;
  return true;
}

When it's large code model, and MO.isGlobal() == true, GlobalToc is definitely true, so there is no situation that when MO.isGlobal() &&GlobalToc == false, but the code model is not large.

Xiangling_L marked 2 inline comments as done.Thu, Oct 10, 8:37 AM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
776

typo: not large --> large

hubert.reinterpretcast marked an inline comment as done.

LGTM with some straightforward changes (can be fixed on the commit).

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
687

My fault on the previous comment; please adjust the space: const MCSymbol *const MOSymbol.

776

Okay, thanks.

1379

I prefer to use meaningful names:

for (const auto &TOCMapPair : TOC) {
  const MCSymbol *const TOCEntryTarget = TOCMapPair.first;
  MCSymbol *const TOCEntryLabel = TOCMapPair.second;

  OutStreamer->EmitLabel(TOCEntryLabel);
  if (isPPC64) {
    TS.emitTCEntry(*TOCEntryTarget);
  } else {
    OutStreamer->EmitValueToAlignment(4);
    OutStreamer->EmitSymbolValue(TOCEntryTarget, 4)
  }
}
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
515

s/the machine operand/a machine operand for a TOC pseudo-machine instruction/;

Xiangling_L marked 2 inline comments as done.Thu, Oct 10, 9:14 AM
Xiangling_L closed this revision.Thu, Oct 10, 11:58 AM