[PATCH] D45961: [MC] Add MCSubtargetInfo to MCAlignFragment [NFC]

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

In https://reviews.llvm.org/D45961#1200623, @asb wrote:

> You will also need to patch at least clang tools/driver/cc1as_main.cpp due to the change to InitSections.
>
> Mechanically the changes seem fine. There's one aspect I'd appreciate further comment on: having to perform getLastSubtargetInfo seems pragmatic but feels ugly. Are there cases where this might be incorrect? Should there be a FIXME indicating that some superior solution for ensuring access to an appropate MCSubtargetInfo should be implemented?


Thanks for the review. I think that in the context that it is used getLastSubtargetInfo() is at, however that doesn't mean that it could potentially be used in a different context in the future.

- For a section with ConstantPools we are guaranteed to have at least one MCFragment with a valid MCSubtarget as there must exist a fragment with an instruction that needs a constant pool entry. In theory given that constant-pools are not instructions it shouldn't actually matter what the encoding of the NOP is because there will most likely be a crash if control flow gets to the constant pool.
- For the Mips RoundSectionSizes option then if the last fragment has instructions we can use its MCSubtarget for the NOP and if not then we can use the per translation unit MCSubtarget. Moreover Mips doesn't even use the Subtarget when determining the NOP encoding anyway.

With this in mind it may be better to change the code so that getLastSubtargetInfo() is no longer required.

- In ConstantPools.cpp::emitEntries() don't use EmitCodeAlignment(), use EmitValueToAlignment() instead. As mentioned above valid code shouldn't ever fall through the padding into the constant pool.
- In Mips we can just use the global STI as there is only one NOP encoding anyway.

I don't think that there are many alternative implementations unless we make the getSubtargetInfo() return a pointer rather than a reference. I'm not sure if that would be a change worth making.

If it sounds ok to you, I can post another patch without getLastSubtargetInfo()?

One possible option might be to fix the surrounding code so that getLastSubtargetInfo() isn't needed.


https://reviews.llvm.org/D45961