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

[Hsiangkai Wang via Phabricator via llvm-commits]

HsiangKai added inline comments.


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


================
Comment at: llvm/test/CodeGen/RISCV/codemodel-lowering.ll:138
 ; RV32I-MEDIUM:       # %bb.0:
-; RV32I-MEDIUM-NEXT:  .LBB3_1: # Label of block must be emitted
-; RV32I-MEDIUM-NEXT:    auipc a1, %pcrel_hi(.LCPI3_0)
-; RV32I-MEDIUM-NEXT:    addi a1, a1, %pcrel_lo(.LBB3_1)
-; RV32I-MEDIUM-NEXT:    flw ft0, 0(a1)
-; RV32I-MEDIUM-NEXT:    fmv.w.x ft1, a0
-; RV32I-MEDIUM-NEXT:    fadd.s ft0, ft1, ft0
+; RV32I-MEDIUM-NEXT:    fmv.w.x ft0, a0
+; RV32I-MEDIUM-NEXT:  .LPCRelHi3:
----------------
Here is an example that to change isPosition() will change instruction scheduling behavior. After the patch, auipc will become the boundary of scheduling regions. fmv.w.x will become the only instruction in its own region.  If there are multiple latencies in load-to-use in the microarchitecture, fmv.w.x will not be able to fill the latencies after flw. It will have negative impact on the performance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123264