[AIX][XCOFF]emit extern linkage for the llvm intrinsic symbol

[DiggerLin]

Summary

1. when we call memset, memcopy,memmove etc(this are llvm intrinsic function) in the c source code. the llvm will generate IR

like call call void @llvm.memset.p0i8.i32(i8* align 4 bitcast (%struct.S* @s to i8*), i8 %1, i32 %2, i1 false)
for c source code
bash> cat test_memset.call

struct S{
 int a;
 int b;
};
extern struct  S s;
void bar() {
  memset(&s, s.b, s.b);
}

like

%struct.S = type { i32, i32 }
@s = external global %struct.S, align 4
; Function Attrs: noinline nounwind optnone
define void @bar() #0 {
entry:
  %0 = load i32, i32* getelementptr inbounds (%struct.S, %struct.S* @s, i32 0, i32 1), align 4
  %1 = trunc i32 %0 to i8
  %2 = load i32, i32* getelementptr inbounds (%struct.S, %struct.S* @s, i32 0, i32 1), align 4
  call void @llvm.memset.p0i8.i32(i8* align 4 bitcast (%struct.S* @s to i8*), i8 %1, i32 %2, i1 false)
  ret void
}
declare void @llvm.memset.p0i8.i32(i8* nocapture writeonly, i8, i32, i1 immarg) #1

If we want to let the aix as assembly compile pass without -u
it need to has following assembly code.
.extern .memset
(we do not output extern linkage for llvm instrinsic function.
even if we output the extern linkage for llvm intrinsic function, we should not out .extern llvm.memset.p0i8.i32,
instead of we should emit .extern memset)

2. for other llvm buildin function floatdidf . even if we do not call these function floatdidf in the c source code(the generated IR also do not the call __floatdidf . the function call

was generated in the LLVM optimized.
the function is not in the functions list of Module, but we still need to emit extern .__floatdidf

The solution for it as :
We record all the lllvm intrinsic extern symbol when transformCallee(), and emit all these symbol in the AsmPrinter::doFinalization(Module &M)

[jasonliu]

llvm/include/llvm/MC/MCContext.h

105 ↗

(On Diff #260321)

Just a general comment on the data structure that's using here.
Have we thought about using using a SmallPtrSet so that you don't need to worry about inserting the duplicated value into the vector?

423 ↗

(On Diff #260321)

Is it necessary to have wrappers to the real data structure?
I feel like returning the actual data structure here is not that bad. And user of this data structure have much more freedom using something like range-based for loop, instead of the iterator interface.

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

19 ↗

(On Diff #260321)

nit: remove this Attr.

[DiggerLin]

llvm/include/llvm/MC/MCContext.h

105 ↗

(On Diff #260321)

/ SmallPtrSet - This class implements a set which is optimized for holding
/ SmallSize or less elements. This internally rounds up SmallSize to the next
/ power of two if it is not already a power of two. See the comments above
/ SmallPtrSetImplBase for details of the algorithm.
template<class PtrType, unsigned SmallSize>
class SmallPtrSet : public SmallPtrSetImpl<PtrType> {

// In small mode SmallPtrSet uses linear search for the elements, so it is
// not a good idea to choose this value too high. You may consider using a
// DenseSet<> instead if you expect many elements in the set.
static_assert(SmallSize <= 32, "SmallSize should be small");

There are hundreds llvm intrinsic function. I will use std::set

[DiggerLin]

address comment

[jasonliu]

llvm/include/llvm/MC/MCContext.h

105 ↗

(On Diff #260321)

There are hundreds of llvm intrinsic functions. But are you expecting every compilation will contain hundreds of llvm intrinsic functions? I would say most of the time, you would see those less than 8intrinsic functions in one compilation unit , except for some very heavy mathematic case.

[jasonliu]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1534 ↗

(On Diff #260740)

Ranged based for loop?

[hubert.reinterpretcast]

llvm/include/llvm/MC/MCContext.h

105 ↗

(On Diff #260321)

I agree with @jasonliu, and even if SmallPtrSet is too small, why not use DenseSet like the comments say?

[DiggerLin]

address comment

llvm/include/llvm/MC/MCContext.h

105 ↗

(On Diff #260321)

I think we can not guarantee the llvm intrinsic sysbols in a compilation unit is less than 32. I will use DenseSet.

[DiggerLin]

using SmallPtrSet as comment

[sfertile]

Have you considered not transforming ExternalSymbolSDNodes, but instead supporting them for AIX and then creating the symbol and emitting the linkage for the ExternalSymbol machine operand in the ASMPrinter? The approach this patch takes adds complexity to a target independent part of the MC infrastructure. The amount of complexity it adds is not unreasonable, but I think if we can contain the complexity to the PowerPC backend its worth considering strongly even if the implementation there is more difficult.

[DiggerLin]

upload wrong diff . discard previous diff

[jhenderson]

llvm/include/llvm/MC/MCContext.h

104 ↗

(On Diff #260934)

Nit: symbol's -> symbols' (or if you don't like that, try "the linkage of these symbols.")

I would write emiting as emitting, but I don't know if that's a British versus American English thing.

Here and in other comments, should "llvm" be "LLVM"?

[DiggerLin]

store llvm intrinsic symbols in the PPCAIXAsmPrinter

[jhenderson]

Please reupload the latest patch with context.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

152 ↗

(On Diff #261728)

Should it be spelt XCOFF?

1756 ↗

(On Diff #261728)

Don't use auto here. Also, should this really be a copy and not a const reference?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5284 ↗

(On Diff #261728)

Should this be const MCSymbolXCOFF *Sym?

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

2 ↗

(On Diff #261728)

Please don't break the lines up like this. I'd put -mattr=altivec on the same line as the rest of the llc line, but if you do want to break it onto a new line, split it roughly halfway through the arguments, and indent the second line, then put the FileCheck line on a new line. Same goes below.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1756 ↗

(On Diff #261728)

it store the a pointer of MCSymbol in the IntrinsicSymbols. I think return a copy of pointer is same efficient as const reference.

and the

bool MCXCOFFStreamer::emitSymbolAttribute(MCSymbol *Sym,
                                          MCSymbolAttr Attribute)

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5284 ↗

(On Diff #261728)

thanks

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

2 ↗

(On Diff #261728)

thanks

[DiggerLin]

address comment

[jasonliu]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5287 ↗

(On Diff #262097)

If there exists a user-declared function whose name is the same as the
ExternalSymbol's, then we pick up the user-declared version.

How do we handle this scenario under the new logic? Are we still able to pick up the user-declared version without generating .extern for that function?

[jasonliu]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5287 ↗

(On Diff #262097)

Please consider the case in llvm/test/CodeGen/PowerPC/aix-user-defined-memcpy.ll

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1756 ↗

(On Diff #261728)

One issue with auto here is that it's not obvious that we have a pointer type (and even knowing how Sym is used does not rule out the possibility that it is merely convertible to the pointer type). Using auto * might not have caused the comment to be raised, but since the comment has been raised, please get rid of the auto.

[DiggerLin]

address comment

[jhenderson]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1756 ↗

(On Diff #261728)

By the way, there's a related style guide section on usage of auto: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

2 ↗

(On Diff #261728)

One more related request: Indent the FileCheck directive by an extra two spaces to make it obvious it's a continuation of the previous line:

; RUN: llc ... | \
; RUN:   FileCheck ...

Same below.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1756 ↗

(On Diff #261728)

thanks for the information.

[jasonliu]

It's a bit awkward to use the emitInstruction function to record the special functions, as it is actually not doing anything related to "emit instruction".
But I'm not sure if there is any better way to do it if we want to hide the logic in PPCAsmPrinter instead of exposing it in target independent MCContext.
I will see what other people think.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5295 ↗

(On Diff #262155)

Could we have a lambda function to return a MCSymbolXCOFF with the correct name for it? Then replace the similar logic in getAIXFuncEntryPointSymbolSDNode with this lambda function call as well.
We should limit the places to do the "." + func_name dance.

[sfertile]

👍 Thanks for trying this out Digger, I think this approach is the right one to take. I've had an initial look and left a few comments inline. I'll need to spend some time understanding ExternalSymbolSDNodes a bit better before being able to review the patch thoroughly enough to approve.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

152 ↗

(On Diff #261728)

IIUC this isn't directly related to intrinsics, but to ExternalSymbolSD nodes, with the lowering of intrinsics being just one of the ways we can end up with an ExternalSymbolSDNodes. Intrinsic lowering might be the only way we expect to get one of these nodes on AIX right now but that can change in the future. The naming and comments need to reflect that this is explicitly for handling emitting the linkage of ExternalSymbolSDNodes.

1726 ↗

(On Diff #262155)

Choosing to implement this override and handle here instead of adding to the base classes emitInstruction makes this really nice and clean. Good job.

1733 ↗

(On Diff #262155)

What is the state of tail-calls on AIX? I know we can't tail call in many of the situations where we can on Linux, however if they aren't completely disabled we might need to add the tail-call opcodes here for similar transformation.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5283 ↗

(On Diff #262155)

I'm not too familiar with when we get ExternalSymbolSDNodes or what the semantic should be if we have an ExternalSymbolSDNode and a IR declaration for the same function, but it seems odd to treat the situation differently based on XCOFF vs ELF. @Xiangling_L Was aix-user-defined-memcpy.ll motivated by real code? If so I'd like to investigate it a bit more.

5294 ↗

(On Diff #262155)

Why not leave the name as is, and prepend the '.' when we create/lookup the symbol in the AsmPrinter?

[jasonliu]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5283 ↗

(On Diff #262155)

It seems for real C code. If there is a user-provided memset/memcpy, clang would transform the call to that user-provided memset/memcpy instead of generating llvm.memset/memcpy intrinsic.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5294 ↗

(On Diff #262155)

if leave name as is, the branch will be
bl memset
not
bl .memset

if leave name as is , we want to generate bl .memset . we need to modify the operand(ExternalSymbol) of branch to generate correct bl .memset

[DiggerLin]

address comment

[sfertile]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1734 ↗

(On Diff #262677)

We haven't implemented TLS on AIX yet, and I'm not sure which if any of these call pseudos will be used. How about adding them as a separate case with a report_fatal_error which can be revisited as part of TLS implementation on AIX.

152 ↗

(On Diff #261728)

Sorry I wasn't more explicit in my previous comment. I don't think we should be using the name 'intrinsic' here at all.

/// Symbols lowered from ExternalSymbolSDNodes.
SmallPtrSet<MCSymbol *, 8> ExternalSymbols;

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5264 ↗

(On Diff #262677)

I'm don't think this is what Jason meant from his comment.

5294 ↗

(On Diff #262155)

Yes, which is why I said you would move where you prepend the '.' from here, to where you create the MCSymbol from the MO_ExternalSymbol in the ASM printer.

I think our handling of when there is deceleration in the IR matching the ExtrernalSymbol name is wrong, and it should either be done for all subtargets, or none (but I'm not 100% sure of that yet and investigating). If they are to be common-ed, then by moving modifying the symbols name too the ASMprinter, it lets us have identical code for all subtargets for transforming ExternalSymbolSDNode callees.

I'd say hold off on making this change until we better understand the IR Decl/ExternalSym situation though.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1734 ↗

(On Diff #262677)

I will delete the TLS related opcode.

152 ↗

(On Diff #261728)

thanks

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5264 ↗

(On Diff #262677)

thanks.

5294 ↗

(On Diff #262155)

Discussed with Sean offline, I will keep the implement.

[DiggerLin]

address comment

[sfertile]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5294 ↗

(On Diff #262155)

Expanding a bit: I though we could create the MCSymbol in PPCAIXAsmPrinter::emitInstruction and then create a new MCSymbol MachineOperand and change the operand to that. Digger explained how I was mistaken and we can not change the operand on the MachineInstr, so we have to stick with this approach.

[sfertile]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1733 ↗

(On Diff #262155)

You have added PPC::TAILB so I am assuming that means we do support tail-calls on AIX. Why do we not need to add any of the other tail call pseudos. At the very least i would expect we need PPC::TAILB8 for 64-bit support if we expect TAILB for 32-bit. What about TAILBA[8] or TAILBCTR[8]?

153 ↗

(On Diff #263179)

Not all intrinsics get handled this way, and not all of these symbols necessarily come from lowering an intrinsic: so we shouldn't be using 'Intrinsic' in the naming. I suggested 'ExternalSymbols' above but that sprobably too generic.

[Xiangling_L]

Can you also please update the testcase aix-user-defined-memcpy.ll to include:
CHECK-NOT: .extern .memcpy

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5283 ↗

(On Diff #262155)

Just as a record. As we discussed offline, a real C code is like:

int memcpy ( void * destination, int num ) { return 3; }
typedef struct cpy_array {
  int array[20];
} cpy;
int main() {
  cpy A = {1,2,3,4,5,6,7,8,9,10, 11, 12, 13, 14, 15,16};
  cpy B = A;
  return B.array[0];
}

And Clang FE will generate the following IR for AIX:

; Function Attrs: noinline nounwind optnone
define i32 @memcpy(i8* %destination, i32 %num) #0 {
entry:
  %destination.addr = alloca i8*, align 4
  %num.addr = alloca i32, align 4
  store i8* %destination, i8** %destination.addr, align 4
  store i32 %num, i32* %num.addr, align 4
  ret i32 3
}
; Function Attrs: noinline nounwind optnone
define i32 @main() #0 {
entry:
  %retval = alloca i32, align 4
  %A = alloca %struct.cpy_array, align 4
  %B = alloca %struct.cpy_array, align 4
  store i32 0, i32* %retval, align 4
  %0 = bitcast %struct.cpy_array* %A to i8*
  call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 %0, i8* align 4 bitcast (%struct.cpy_array* @__const.main.A to i8*), i32 80, i1 false)
  %1 = bitcast %struct.cpy_array* %B to i8*
  %2 = bitcast %struct.cpy_array* %A to i8*
  call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 %1, i8* align 4 %2, i32 80, i1 false)
  %array = getelementptr inbounds %struct.cpy_array, %struct.cpy_array* %B, i32 0, i32 0
  %arrayidx = getelementptr inbounds [20 x i32], [20 x i32]* %array, i32 0, i32 0
  %3 = load i32, i32* %arrayidx, align 4
  ret i32 %3
}

llvm/test/CodeGen/PowerPC/aix-cc-byval-mem.ll

91 ↗

(On Diff #263179)

Though unrelated to this patch, I am wondering if we can replace & to something more meaningful like extsym etc. like what we have for MCSymbol as mcsymbol?

[sfertile]

llvm/test/CodeGen/PowerPC/aix-cc-byval-mem.ll

91 ↗

(On Diff #263179)

Good suggestion, send a quick email to the llvm-dev list and see if there interest for this, or any objections.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1733 ↗

(On Diff #262155)

as discussed with Sean offline, since wo do not support tail call for the extern symbol now . I added a report_fatal_error("Tail call for extern symbol not yet implemented.");

153 ↗

(On Diff #263179)

thanks . change to "ExtSymSDNodeSymbols" as you discussed on the slack

[DiggerLin]

address comment

[jhenderson]

llvm/test/CodeGen/PowerPC/aix-user-defined-memcpy.ll

11–12 ↗

(On Diff #266075)

I know this is what was done elsewhere in the test, but I don't think we should blindly copy bad practice here, and instead you should just reflow the line like I suggested in the other test. If you do think the llc line is too long and needs to broken up, I'd recommend the following:

; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff \
; RUN:     -mcpu=pwr4 -mattr=-altivec < %s | \
; RUN:   FileCheck %s

[jhenderson]

No more style/test comments from me now. I don't know enough about other things to further review this.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1734 ↗

(On Diff #262677)

Can we have an update on why the TLS call pseudos were simply removed instead of being tracked by report_fatal_error as @sfertile suggested? I think that leaving some breadcrumbs in the code would be useful.

1763 ↗

(On Diff #266563)

Please use the immediate base class name to refer to the base class implementation.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5244 ↗

(On Diff #266563)

Should the name be adjusted to match that of the function introduced by https://reviews.llvm.org/D80402? @jasonliu?

llvm/test/CodeGen/PowerPC/aix-cc-byval-mem.ll

91 ↗

(On Diff #263179)

@sfertile, do we have further news on this?

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

8 ↗

(On Diff #266563)

We've been indenting options to line up with options on the previous line. That usually avoids indenting by only two spaces (which we may want to reserve for indentation after things like shell operators).

12 ↗

(On Diff #266563)

Same comment.

[jasonliu]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5244 ↗

(On Diff #266563)

Yeah, I think matching them would be useful for grepping.

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

42 ↗

(On Diff #266563)

Start with COMMON-LABEL with this line and omit the lines above. The existence of the function descriptor is not the purpose of this test. After this, there would be no difference between 32-bit and 64-bit and we should be able to replace the prefix with plain CHECK.

45 ↗

(On Diff #266563)

I think the object code generation path test needs to verify that the relocation refers to the correct symbol table entry.

51 ↗

(On Diff #266563)

I think we should similarly omit the lines in this block before this last one.

74 ↗

(On Diff #266563)

I think we can omit the symbol table entries aside from the one for .memset.

[DiggerLin]

address comment

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5244 ↗

(On Diff #266563)

I do not think we can use MCSymbol *getFunctionEntryPointSymbol(const Function *F,const TargetMachine &TM) function , for the .memset or memcpy, we do not have Function object for it.

[jasonliu]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5244 ↗

(On Diff #266563)

I don't think you can actually call that function, as the function is in TLOF and you do not have access to that from here.
Hubert and I are talking about matching the name.

[jhenderson]

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

9–11 ↗

(On Diff #267645)

You can fold these two lines into one. No need for them to be separate:

; RUN: llvm-readobj --symbols --relocations --expand-relocs %t.o | \
; RUN:   FileCheck --check-prefix=SYMRELOC %s

[DiggerLin]

address comment

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

9–11 ↗

(On Diff #267645)

Thanks for your suggestion, but I think separating in two is based is more reasonable ,One test for test Symbol. other test for test relocation. it is more readable.

[jhenderson]

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

9–11 ↗

(On Diff #267645)

I'm not sure how it's more readable. It's very clear from my suggested command-line that we want to test the symbols and relocations (otherwise the options wouldn't be present), and would reduce the amount of work required to figure out which checks below correspond to which FileCheck line.

It would also be more in keeping with most tests I've worked with, and would reduce the number of executable invocations from 4 to 2, improving test run time.

[DiggerLin]

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

9–11 ↗

(On Diff #267645)

I tried it with llvm-readobj --symbols --relocations --expand-relocs
it display the relocation information first and than symbol information.

in the test case we want to verify symbol first and than relocation.
I have to keep current test.

[DiggerLin]

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

9–11 ↗

(On Diff #267645)

@hubert.reinterpretcast , what is your option on combine the

llvm-readobj --symbols

and

llvm-readobj --relocations --expand-relocs

into

llvm-readobj --symbols --relocations --expand-relocs

and than verify relocation information first as

; SYMRELOC: Relocation {{[{][[:space:]] *}}Virtual Address: 0x18
; SYMRELOC-NEXT: Symbol: .memset (0)
; SYMRELOC-NEXT: IsSigned: Yes
; SYMRELOC-NEXT: FixupBitValue: 0
; SYMRELOC-NEXT: Length: 26
; SYMRELOC-NEXT: Type: R_RBR (0x1A)
; SYMRELOC-NEXT: }

; SYMRELOC: Symbol {
; SYMRELOC-NEXT: Index: 0
; SYMRELOC-NEXT: Name: .memset
; SYMRELOC-NEXT: Value (RelocatableAddress): 0x0
; SYMRELOC-NEXT: Section: N_UNDEF
; SYMRELOC-NEXT: Type: 0x0
; SYMRELOC-NEXT: StorageCl
......

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

32 ↗

(On Diff #267932)

I believe I requested CHECK-LABEL: here.

34 ↗

(On Diff #267932)

Please indent the instructions in relation to the label.

60 ↗

(On Diff #267932)

Moot point, but this should be aligned to match the closing brace.

9–11 ↗

(On Diff #267645)

I suggest using llvm-objdump -d --symbol-description for the relocation check. As it is, we can't tell whether the relocation we found is in the right place. This happens to make it so that we end up with two separate tool invocations.

[jasonliu]

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

9–11 ↗

(On Diff #267645)

I think you meant: llvm-objdump -d -r --symbol-description

[jhenderson]

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

9–11 ↗

(On Diff #267645)

It's moot, given the llvm-objdump suggestion, but I'm not sure it really matters which order symbols or relocations are printed in.

[DiggerLin]

llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll

9–11 ↗

(On Diff #267645)

because there is symbol index in the relocation information. we prefer to verify the symbol first and then relocation. otherwise, when we verified relocation, we have to go down the test file to search the symbol information.

[DiggerLin]

change test case based on comment

[sfertile]

Hey Digger, sorry for letting this sit for so long. Feel free to ping reviews periodically to get peoples attention back on them.

Other then a few minor nits, LGTM.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

152 ↗

(On Diff #268466)

Minor nit: Mention that we will need the emit extern linkage for them in the comment.

1800 ↗

(On Diff #268466)

nit: I would instead say 'not supported' or 'not expected'. The AIX ABI makes a tail-call to an external symbol really unexpected.

llvm/test/CodeGen/PowerPC/aix-cc-byval-mem.ll

91 ↗

(On Diff #263179)

Not sure, I suggested @Xiangling_L send an email to the dev list to discuss, but never followed whether it happened or not. Either way I think its outside the scope of this patch.

[Xiangling_L]

llvm/test/CodeGen/PowerPC/aix-cc-byval-mem.ll

91 ↗

(On Diff #263179)

Sorry, I think there were some miscommunications. I thought you would send the email. Thanks for your suggestion and will do.

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

 
 struct XCOFFSymbolInfo {
   Optional<XCOFF::StorageMappingClass> StorageMappingClass;
   Optional<uint32_t> Index;
   bool IsLabel;
   XCOFFSymbolInfo(Optional<XCOFF::StorageMappingClass> Smc,
                   Optional<uint32_t> Idx, bool Label)
       : StorageMappingClass(Smc), Index(Idx), IsLabel(Label) {}
+
+  bool operator<(const XCOFFSymbolInfo &SymInfo) const;
 };
 
 struct SymbolInfoTy {
   uint64_t Addr;
   StringRef Name;
   union {
     uint8_t Type;
     XCOFFSymbolInfo XCOFFSymInfo;
SymbolInfoTy(uint64_t Addr, StringRef Name, uint8_t Type)
       : Addr(Addr), Name(Name), Type(Type), IsXCOFF(false) {}
   bool isXCOFF() const { return IsXCOFF; }
 
 private:
   friend bool operator<(const SymbolInfoTy &P1, const SymbolInfoTy &P2) {
     assert(P1.IsXCOFF == P2.IsXCOFF &&
            "P1.IsXCOFF should be equal to P2.IsXCOFF.");
     if (P1.IsXCOFF)
-      return std::tie(P1.Addr, P1.Name) < std::tie(P2.Addr, P2.Name);
-    else
-      return std::tie(P1.Addr, P1.Name, P1.Type) <
+      return std::tie(P1.Addr, P1.XCOFFSymInfo, P1.Name) <
+             std::tie(P2.Addr, P2.XCOFFSymInfo, P2.Name);
+
              std::tie(P2.Addr, P2.Name, P2.Type);
   }
 };
 
 using SectionSymbolsTy = std::vector<SymbolInfoTy>;
 
 template <typename T> class ArrayRef;
 class MCContext;

llvm/lib/MC/MCDisassembler/MCDisassembler.cpp

void MCDisassembler::tryAddingPcLoadReferenceComment(int64_t Value,
                                                      uint64_t Address) const {
   if (Symbolizer)
     Symbolizer->tryAddingPcLoadReferenceComment(*CommentStream, Value, Address);
 }
 
 void MCDisassembler::setSymbolizer(std::unique_ptr<MCSymbolizer> Symzer) {
   Symbolizer = std::move(Symzer);
 }
+
+#define SMC_PCASE(A, P)                                                         \
+  case XCOFF::XMC_##A:                                                         \
+    return P;
+
+uint8_t getSMCPriority(XCOFF::StorageMappingClass SMC) {
+  switch (SMC) {
+    SMC_PCASE(PR, 1)
+    SMC_PCASE(RO, 1)
+    SMC_PCASE(DB, 1)
+    SMC_PCASE(GL, 1)
+    SMC_PCASE(XO, 1)
+    SMC_PCASE(SV, 1)
+    SMC_PCASE(SV64, 1)
+    SMC_PCASE(SV3264, 1)
+    SMC_PCASE(TI, 1)
+    SMC_PCASE(TB, 1)
+    SMC_PCASE(RW, 1)
+    SMC_PCASE(TC0, 0)
+    SMC_PCASE(TC, 1)
+    SMC_PCASE(TD, 1)
+    SMC_PCASE(DS, 1)
+    SMC_PCASE(UA, 1)
+    SMC_PCASE(BS, 1)
+    SMC_PCASE(UC, 1)
+    SMC_PCASE(TL, 1)
+    SMC_PCASE(UL, 1)
+    SMC_PCASE(TE, 1)
+#undef SMC_PCASE
+  }
+  return 0;
+}
+
+/// The function is for symbol sorting when symbols have the same address.
+/// The symbols in the same section are sorted in ascending order.
+/// llvm-objdump -D will choose high priority symbol to display when there are
+/// symbols with the same address.
+
+bool XCOFFSymbolInfo::operator<(const XCOFFSymbolInfo &SymInfo) const {
+  // Compare Label first. If the symbol is not a label, it is lower than label
+  // symbol.
+  if (IsLabel != SymInfo.IsLabel)
+    return SymInfo.IsLabel;
+
+  // If symbol does not have a StorageMappingClass, it is lower priority than a
+  // symbol which has a StorageMappingClass.
+  if (!StorageMappingClass)
+    return true;
+
+  if (!SymInfo.StorageMappingClass)
+    return false;
+
+  return getSMCPriority(StorageMappingClass.getValue()) <
+         getSMCPriority(SymInfo.StorageMappingClass.getValue());
+}

llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbol-description.test

 ##
 ## int func(void)  {
 ##   return a;
 ## }
 
 COMMON: Inputs/xcoff-section-headers.o: file format aixcoff-rs6000
 COMMON: Disassembly of section .text:
 PLAIN:      00000000 <.text>:
-DESC:       00000000 (idx: 4) .text:
+DESC:       00000000 (idx: 16) .func: 
 COMMON-NEXT:        0: 80 62 00 04                    lwz 3, 4(2)
 COMMON-NEXT:        4: 80 63 00 00                    lwz 3, 0(3)
 COMMON-NEXT:        8: 4e 80 00 20                    blr
 COMMON-NEXT:        c: 00 00 00 00                    <unknown>
 COMMON-NEXT:       10: 00 00 20 40                    <unknown>
 COMMON-NEXT:       14: 00 00 00 01                    <unknown>
 COMMON-NEXT:       18: 00 00 00 0c                    <unknown>
 COMMON-NEXT:       1c: 00 04 66 75                    <unknown>