[PATCH] D132818: [LoongArch] Fixup value adjustment in applyFixup

Tue Aug 30 20:43:11 PDT 2022

xen0n added inline comments.

================
Comment at: llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp:189-190
   MCFixupKindInfo Info = getFixupKindInfo(Kind);
-  // TODO: Apply any target-specific value adjustments.
+  if (!Value)
+    return; // Doesn't change encoding.
+  // Apply any target-specific value adjustments.
----------------
wangleiat wrote:
> xen0n wrote:
> > This is placed here due to review comment on D132323, does the situation change enough to make this change appropriate?
> Personal preference, just put the processing of `Value` together (Increases the readability of the code). Maybe it was put in the front at the time because of performance? If you think this is not good, I can modify it back.
Granted it's just a teensy bit of performance, but it's still @SixWeining's input after all, and after the change it's back to the same as RISCV, and effectively code churn for no objective reason. So I'd personally prefer the 2 lines be moved back. Thanks!

================
Comment at: llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp:266
         break;
+      case LoongArch::B:
+        FixupKind = LoongArch::fixup_loongarch_b26;
----------------
wangleiat wrote:
> xen0n wrote:
> > Do we want to also handle `BL` here? Given it's the only other insn of the format.
> > Do we want to also handle `BL` here? Given it's the only other insn of the format.
> 
> It is not needed. This is because `BL` are handled specifically in both the `LowerCall` phase and `ASMParser`.
Okay, thanks for the explanation. I have no problem with that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132818