[PATCH] D123264: [RISCV] Pre-RA expand pseudos pass

[luxufan via Phabricator via llvm-commits]

StephenFan added a comment.

I'm also curious why we need



================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1212
+    return isLabel() || isCFIInstruction() || getPreInstrSymbol() ||
+           getPostInstrSymbol();
+  }
----------------
HsiangKai wrote:
> luismarques wrote:
> > HsiangKai wrote:
> > > luismarques wrote:
> > > > HsiangKai wrote:
> > > > > Is it enough to only add checking of getPreInstrSymbol()?
> > > > What do you mean, enough for what? (This patch also checks `getPostInstrSymbol()`).
> > > > In my testing that was enough to ensure that the label always pointed to the correct upper instruction, but feedback about this is welcome.
> > > Just curious why checking getPostInstrSymbol() here. There is no setPostInstrSymbol() in this patch.
> > It was just for regularity. My reasoning was that if `some_label: INSTR;` is a position then `INSTR; some_label:` also is (assuming the label is attached to the INSTR), so this change made sense for both. But it's not necessary for *this* particular patch. Thanks for checking that oddity :)
> To change the condition of isPosition() will have side effects in other passes. One example is instruction scheduling. isPosition() is one of the condition to decide region boundaries for instruction scheduling. Refer to https://github.com/llvm/llvm-project/blob/be5c15c7aee1ef4964c88031555ed6b0f59ebc23/llvm/lib/CodeGen/TargetInstrInfo.cpp#L1025
> What do you mean, enough for what? (This patch also checks `getPostInstrSymbol()`).
> In my testing that was enough to ensure that the label always pointed to the correct upper instruction, but feedback about this is welcome.

I'm curious about by `setPreInstrSymbol`, the compiler still can't ensure that the label always pointed
to the correct upper instruction? I think after `setPreInstrSymbol`, the AsmPrinter will always emit the symbol label before this instruction. 


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:458
+      BuildMI(MBB, MBBI, DL, TII->get(RISCV::AUIPC), ScratchReg).add(Symbol);
+  MIAUIPC->setPreInstrSymbol(*MF, AUIPCSymbol);
+
----------------
Because we have set a pre-instr symbol and this symbol is unique, I think we need to make the AUIPC instruction not duplicable. If `auipc` instruction were duplicated, the unique symbol would also have multiple definitions. It is in conflict with the symbol is unique.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123264