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

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

luismarques added inline comments.


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

Indeed. There was an issue where the label for the upper instruction was being removed, and it was seemingly caused by the branch folder pass. I think I misinterpreted what was going on there, possibly because I was originally using a different implementation and got confused. When I looked again into that it seemed that the invalid transformation made sense if two AUIPCs were actually the same. They were, except for the pre and post-instruction symbols. Sure enough, extending the MI comparison to check for the pre- and post-instruction symbols solved the issue without messing with the `isPosition` predicate. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123264