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

[Luís Marques via Phabricator via llvm-commits]

luismarques marked 3 inline comments as done.
luismarques added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:454
+  Symbol.setTargetFlags(FlagsHi);
+  MCSymbol *AUIPCSymbol = MF->getContext().createTempSymbol("PCRelHi");
+
----------------
StephenFan wrote:
> Using `createTempSymbol` to create a symbol will cause the object code's string table to have an empty name for the symbol. Maybe you could use the `MF.getPICBaseSymbol()` function to create the PIC base symbol. It can create the function local symbol by using the unique function id.
I'll look into this more carefully later. I thought (but didn't verify) that `createNamedTempSymbol` would emit the symbol like you wanted, even if that symbol wasn't then function-local. But then I looked at object files built with the original patch and they seemed already have the `.LPCRelHi0` etc in `.symtab`. So I'm not sure I follow your point. As I said, I need to look into this more carefully.


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:458
+      BuildMI(MBB, MBBI, DL, TII->get(RISCV::AUIPC), ScratchReg).add(Symbol);
+  MIAUIPC->setPreInstrSymbol(*MF, AUIPCSymbol);
+
----------------
StephenFan wrote:
> 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.
That makes sense. It wasn't immediately obvious how to dynamically set the not duplicable flag per instruction instance, so I'll handle this one later. There seemed to be fewer passes that checked the `isNotDuplicable` property than I would expect, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123264