Page MenuHomePhabricator

[AsmPrinter, SystemZ] Allow target to add instructions before section is ended.
ClosedPublic

Authored by jonpa on Sep 9 2021, 8:39 AM.

Details

Summary

SystemZ adds EXRL target instructions in the end of each file. They need to be added to the text section and just like with constant pools this needs to happen before debug info emission since that may end the text section.

In order to facilitate this a new virtual method emitSectionEndings() has been added that is called at the same point as emitConstantPools().

Diff Detail

Event Timeline

jonpa created this revision.Sep 9 2021, 8:39 AM
jonpa requested review of this revision.Sep 9 2021, 8:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2021, 8:39 AM

do you think it'd make sense to merge emitConstantPools into emitSectionEndings?

jonpa updated this revision to Diff 371848.Sep 10 2021, 2:52 AM

do you think it'd make sense to merge emitConstantPools into emitSectionEndings?

How about moving emitConstantPools() into the default implementation of emitSectionEndings() like this..?

do you think it'd make sense to merge emitConstantPools into emitSectionEndings?

How about moving emitConstantPools() into the default implementation of emitSectionEndings() like this..?

Hmm ... that makes me wonder: can we instead just use the existing emitConstantPools in the SystemZ back end to output the EXRL instructions? They are in effect a form of constants ... That shouldn't require any common code changes then.

jonpa added a comment.Sep 10 2021, 5:06 AM

do you think it'd make sense to merge emitConstantPools into emitSectionEndings?

How about moving emitConstantPools() into the default implementation of emitSectionEndings() like this..?

Hmm ... that makes me wonder: can we instead just use the existing emitConstantPools in the SystemZ back end to output the EXRL instructions? They are in effect a form of constants ... That shouldn't require any common code changes then.

I read in the original bug report: "...adding the XC instructions in some other text section out of the eyes of debug is not a good option for z/OS (details excluded)". Therefore I thought they better be placed in the text section...

do you think it'd make sense to merge emitConstantPools into emitSectionEndings?

How about moving emitConstantPools() into the default implementation of emitSectionEndings() like this..?

Hmm ... that makes me wonder: can we instead just use the existing emitConstantPools in the SystemZ back end to output the EXRL instructions? They are in effect a form of constants ... That shouldn't require any common code changes then.

I read in the original bug report: "...adding the XC instructions in some other text section out of the eyes of debug is not a good option for z/OS (details excluded)". Therefore I thought they better be placed in the text section...

Can't a back-end specific emitConstantPools hook just emit those into the .text section? It's called at the same point you proposed new hook would be called, so I thought it should be able to do the same thing ...

jonpa added a comment.Sep 10 2021, 7:55 AM

Can't a back-end specific emitConstantPools hook just emit those into the .text section? It's called at the same point you proposed new hook would be called, so I thought it should be able to do the same thing ...

Ah, I guess that could work.

  • Personally, I would like to see an 'emitSectionEndings()' in AsmPrinter, since there are already are the similar emitStartOfAsmFile() and emitEndOfAsmFile(). It seems to me good to have a clear point before the sections may be closed (and I made the mistake here myself to emit the EXRL target instructions in emitEndOfAsmFile() in the first place...).
  • I also see that emitConstantPools() is called in AsmParser. That should not stop the SystemZ EXRL approach (the EXRLTargets2Sym map should then be empty I think), but it is still somewhat confusing -- I am not sure why that call is needed from there...
  • For a target (like SystemZ) that does not need a custom MCTargetStreamer otherwise it is easier to overload an AsmPrinter method directly.

I could try and see how much extra code it would be to implement a MCTargetStreamer in SystemZ (and move the EXRLTargets2Sym map into it probably), but first I would like your opinions that this would actually be better...

Ah, I see, we've have to implement a MCTargetStreamer first ... In any case, I don't have a strong opinion on this, if the common code change is acceptable, that's of course fine with me. A back-end only solution using a MCTargetStreamer would also be OK.

jonpa updated this revision to Diff 373422.Sep 18 2021, 9:46 AM

Since there does not seem to be any desire for the new hook I proposed and we now have a SystemZTargetStreamer I tried to do this with emitConstantPools() instead.

  • Move EXRLTargets2Sym map from SystemZAsmPrinter to SystemZTargetStreamer.
  • Replace emitEXRLTargetInstructions() with emitConstantPools().

Two minor nits inline, otherwise I agree we should go with that approach now.

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCTargetDesc.cpp
16

Agree with the linter here, please keep the includes sorted alphabetically.

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

Maybe move the assert into the getTargetStreamer() routine, just like in the AsmParser? (In fact, do we really need two copies of the getTargetStreamer() routine now?)

jonpa updated this revision to Diff 373842.Sep 21 2021, 4:12 AM
jonpa marked 2 inline comments as done.

Updated per review.

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

I started out with the assert inside getTargetStreamer(), but then got test failures because the TargetStreamer is not necessarily created in some cases for example when output is discarded. This was while I was still doing this in emitEndOfAsmFile(), but I see that now it is actually possible to move the assert back, which I did.

Hmm, maybe we could store a pointer to it when createAsmTargetStreamer() / createObjectTargetStreamer() is called. I think that maybe it's better to use the getters like now when they are used, as this object is not guaranteed to be present...

uweigand accepted this revision.Sep 21 2021, 4:35 AM

LGTM now, thanks!

This revision is now accepted and ready to land.Sep 21 2021, 4:35 AM
This revision was landed with ongoing or failed builds.Sep 21 2021, 5:35 AM
This revision was automatically updated to reflect the committed changes.