Page MenuHomePhabricator

[PowerPC] Split s34imm into two types
ClosedPublic

Authored by stefanp on Jul 6 2020, 1:22 PM.

Details

Summary

Currently the instruction paddi always takes s34imm as the type for the
34 bit immediate. However, the PC Relative form of the instruction should
not produce the same fixup as the non PC Relative form.
This patch splits the s34imm type into s34imm and s34imm_pcrel so that two
different fixups can be emitted.

Diff Detail

Event Timeline

stefanp created this revision.Jul 6 2020, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2020, 1:22 PM
stefanp added a reviewer: Restricted Project.Jul 6 2020, 1:28 PM
kamaub accepted this revision as: kamaub.Jul 7 2020, 10:05 AM
kamaub added a subscriber: kamaub.

This is LGTM, one suggestion on the clang-format change that can be done pre-commit.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
108

I think it might be a good idea to ignore the clang-format suggestions in this case since the previous way is alot more readable.

This revision is now accepted and ready to land.Jul 7 2020, 10:05 AM

Minor nits, otherwise LGTM.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
108

Yes. Please do not change the existing ones. Unrelated whitespace changes are quite detrimental to meaningful git log history.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp
413

I think we can just skip the switch that does nothing and add an llvm_unreachable for this case.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
466

It seems very odd to me that we would use the _pcrel version here. There should be no way to do anything PC-relative with this instruction since it will necessarily set the PC-Rel bit to zero. The immediate should always be a real immediate (never any fixup).

So although it doesn't matter, we should probably not use the _pcrel version because it will be confusing. I was certainly confused and wrote about 3 versions of this comment :)

nemanjai accepted this revision.Jul 8 2020, 8:18 AM

Forgot to select Accept.

stefanp updated this revision to Diff 276553.Jul 8 2020, 2:12 PM
stefanp marked an inline comment as done.

Fixed the alignment that was auto-fixed.
Removed the switch that was not needed.
Fixed the types in the TD file to be s34imm.

stefanp added inline comments.Jul 8 2020, 2:20 PM
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
466

You are correct.
For some reason I added this here thinking that MLS_DForm_SI34_RT5 was a multi-class just like the one above it... Anyway, I'll fix it.

This revision was automatically updated to reflect the committed changes.
stefanp reopened this revision.Jul 10 2020, 4:03 AM

Going to reopen this review.
The initial commit was pulled due to asserts being hit on an release+asserts build.

This revision is now accepted and ready to land.Jul 10 2020, 4:03 AM
stefanp updated this revision to Diff 281001.Jul 27 2020, 11:47 AM

Changed the way that the error is emitted.

This revision was landed with ongoing or failed builds.Jul 28 2020, 3:56 AM
This revision was automatically updated to reflect the committed changes.