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

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

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


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1212
+    return isLabel() || isCFIInstruction() || getPreInstrSymbol() ||
+           getPostInstrSymbol();
+  }
----------------
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 :)


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:101
   switch (MBBI->getOpcode()) {
   case RISCV::PseudoLLA:
     return expandLoadLocalAddress(MBB, MBBI, NextMBBI);
----------------
HsiangKai wrote:
> Do we still need to handle PseudoLLA in RISCVExpandPseudo?
Possibly not. I'll have to more carefully check if there aren't passes that can reintroduce those instructions. I wonder if it just isn't safer to leave it for now, anyway? In any case, if this patch is accepted and proves stable then it might make sense to eventually move more/all of these expansions to Pre-RA, as that unlocks various optimizations. Sam Elliot had explored that possibility some time ago but he had run into the issue of the instructions and the labels getting out of sync. That seems solved now with my `isPosition` change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123264