[PATCH] D150522: [LoongArch] Support CodeModel::Large codegen

[WÁNG Xuěruì via Phabricator via llvm-commits]

xen0n added a comment.

Thanks for the reviews. I've fixed the bootstrap bug (GOT access gotcha) and will send another revision soon, after the bootstrapped LLVM passes tests.



================
Comment at: llvm/lib/Target/LoongArch/LoongArchExpandPseudoInsts.cpp:211
+
+  unsigned Flags0, Flags1, Flags2, Flags3;
+  switch (IdentifyingMO) {
----------------
benshi001 wrote:
> I think the suffix `s` is unnecessary for variables. :)
Okay, I'll rename these into `MO...` instead, given the source argument is `IdentifyingMO`.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchExpandPseudoInsts.cpp:252
+          : DestReg;
+  Register TmpPart023 =
+      DestReg.isVirtual()
----------------
benshi001 wrote:
> Why name it to `TmpPart023` rather than `TmpPart3` ? 
The name is intended to convey that this temporary value contains the parts 0, 2, and 3 of the desired value (part 1 is loaded with `pcalau12i` to give PC-relative semantics). Which do you think is better, names like "TmpParts023" (notice the plural "parts"), or simpler names like just "TmpA" "TmpB" and "TmpC"?


================
Comment at: llvm/lib/Target/LoongArch/LoongArchExpandPseudoInsts.cpp:309
+    MachineBasicBlock::iterator &NextMBBI, bool Large) {
+  if (Large)
+    return expandLargeAddressLoad(MBB, MBBI, NextMBBI, LoongArch::ADD_D,
----------------
benshi001 wrote:
> Do we also need to add comment about instruction sequence as the following lines ?
The large code model sequence is long, but I'll try describing it with a short sentence anyway. Thanks for the suggestion.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchExpandPseudoInsts.cpp:374
+    MachineBasicBlock::iterator &NextMBBI, bool Large) {
+  if (Large)
+    return expandLargeAddressLoad(MBB, MBBI, NextMBBI, LoongArch::ADD_D,
----------------
benshi001 wrote:
> Do we have to check if it is LA64? Should there be an error if it is LA32+Large ? 
> 
Good idea, will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150522