[PATCH] D83255: [PowerPC] Split s34imm into two types

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

Minor nits, otherwise LGTM.



================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp:97
+        // name                    offset  bits  flags
+        {"fixup_ppc_br24", 6, 24, MCFixupKindInfo::FKF_IsPCRel},
+        {"fixup_ppc_br24_notoc", 6, 24, MCFixupKindInfo::FKF_IsPCRel},
----------------
kamaub wrote:
> 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.
Yes. Please do not change the existing ones. Unrelated whitespace changes are quite detrimental to meaningful git log history.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp:413
+    case PPC::fixup_ppc_imm34:
+      switch (Modifier) {
+      default:
----------------
I think we can just skip the `switch` that does nothing and add an `llvm_unreachable` for this case.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrPrefix.td:399
       def PLI8 : MLS_DForm_SI34_RT5<14, (outs g8rc:$RT),
-                                    (ins s34imm:$SI),
+                                    (ins s34imm_pcrel:$SI),
                                     "pli $RT, $SI", IIC_IntSimple, []>;
----------------
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 :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83255/new/

https://reviews.llvm.org/D83255