Page MenuHomePhabricator

[SystemZ] Recognize mrecord-mcount in backend
ClosedPublic

Authored by jonpa on Dec 17 2019, 2:02 PM.

Details

Reviewers
uweigand
Summary

Emit the mcount_loc_ section with all fentry__calls.

This is done using just raw text like gcc does it (see gcc@06477d3). Is this acceptable?

A check in SystemZDAGToDAGISel::runOnMachineFunction() just like for mnop-mcount to make sure this is only used together with -mfentry.

Diff Detail

Event Timeline

jonpa created this revision.Dec 17 2019, 2:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2019, 2:02 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

This is done using just raw text like gcc does it (see gcc@06477d3). Is this acceptable?

Well, that doesn't really work, since it means the -mrecord-mcount is then simply ignored when using the built-in assembler (i.e. compiling to object code instead of assembler source).

jonpa added a comment.Dec 17 2019, 8:26 PM

This is done using just raw text like gcc does it (see gcc@06477d3). Is this acceptable?

Well, that doesn't really work, since it means the -mrecord-mcount is then simply ignored when using the built-in assembler (i.e. compiling to object code instead of assembler source).

Ah, of course...

jonpa updated this revision to Diff 234451.Dec 17 2019, 8:42 PM

Patch updated to call EmitLabel, SwitchSection() and EmitIntValue() instead of emitting raw text.

I don't know why gcc is emitting a "1:" label... Is it OK to instead emit .Ltmp0:, .Ltmp1: labels (any name) for each fentry?

Not exactly sure exactly what type of section should be created, but calling getELFSection() this way gives the right output, it seems.

Switching back to the previous section causes a ".text" to be printed instead of ".previous".

See mrecord-mcount-01.ll for the output...

uweigand added inline comments.Dec 17 2019, 11:34 PM
llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
568

For consistency, we should really remove the "true" from the "mnop-mcount" attribute as well. (That should be a separate patch.)

577

There's no reason to split the code up this way. You might as well do everything in the block above.

580

This is wrong, you need to emit a reference to the "DotSym" above.

I guess GCC uses the GAS unnamed label feature "1b" -- this is not the hex value 0x1b, but a backward reference to the immediately preceding instance of the "1:" label.

llvm/test/CodeGen/SystemZ/mrecord-mcount-01.ll
13

So instead of ".quad 27" this should read ".quad .Ltmp0".

uweigand added inline comments.Dec 17 2019, 11:47 PM
llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
581

It's probably better to use a OutStreamer->PushSection() / OutStreamer->PopSection() pair here.

jonpa updated this revision to Diff 234565.Dec 18 2019, 9:57 AM
jonpa marked 4 inline comments as done.

Patch updated per review.

Beginning to understand how this is supposed to work now, but still not sure why the text assembly looks ok, while I see with objdump --filetype=obj the new section filled with zeros:

llc llvm/test/CodeGen/SystemZ/mrecord-mcount-01.ll -mtriple=s390x-linux-gnu -mcpu=z10 -o out.o --filetype=obj; objdump -D -z out.o

0000000000000000 <__mcount_loc>:
0:   00 00 00 00             .long   0x00000000
4:   00 00 00 00             .long   0x00000000
8:   00 00 00 00             .long   0x00000000
c:   00 00 00 00             .long   0x00000000

I was expecting to see something like <test1> and <test2> somwhere there... Will the linker fix this somehow (not sure how it could since it's just zeroes...)?

jonpa marked an inline comment as done.Dec 18 2019, 9:58 AM
jonpa added inline comments.
llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
577

Ah, ok.

580

Ah, of course...

Patch updated per review.

Beginning to understand how this is supposed to work now, but still not sure why the text assembly looks ok, while I see with objdump --filetype=obj the new section filled with zeros:

llc llvm/test/CodeGen/SystemZ/mrecord-mcount-01.ll -mtriple=s390x-linux-gnu -mcpu=z10 -o out.o --filetype=obj; objdump -D -z out.o

0000000000000000 <__mcount_loc>:
0:   00 00 00 00             .long   0x00000000
4:   00 00 00 00             .long   0x00000000
8:   00 00 00 00             .long   0x00000000
c:   00 00 00 00             .long   0x00000000

I was expecting to see something like <test1> and <test2> somwhere there... Will the linker fix this somehow (not sure how it could since it's just zeroes...)?

Yes, this will be done via relocations. You should add -r to the objdump command line, then you'll see them :-)

Minor cosmetic comment, otherwise this LGTM.

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
560

I think it would be nicer to have the EmitLabel after the PopSection, so that the label is optically close to the instruction it labels. But that's just cosmetics.

jonpa updated this revision to Diff 234603.Dec 18 2019, 1:15 PM
jonpa marked an inline comment as done.

Updated per review: EmitLabel after PopSection.

SystemZISelDAGToDAG::runOnMachineFunction() refactored to combine the two checks for fentry-call.

uweigand accepted this revision.Dec 18 2019, 2:15 PM

LGTM with minor tweak. Thanks!

llvm/test/CodeGen/SystemZ/mrecord-mcount-01.ll
32

I believe "mop-mcount" no longer needs "true" here now.

This revision is now accepted and ready to land.Dec 18 2019, 2:15 PM
jonpa closed this revision.Dec 19 2019, 9:14 AM
jonpa marked an inline comment as done.