[PATCH] D113439: [RISCV] Add IR intrinsics for reading/write vxrm.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1338
 
+let hasSideEffects = true in {
+def ReadVXRM : ReadSysReg<SysRegVXRM, [VXRM]>;
----------------
craig.topper wrote:
> rogfer01 wrote:
> > Next to the `FRM` above this makes me curious. Either we don't need this or we forgot it for the `FRM` ones?
> > 
> > It is a bit surprising we defined `ReadSysReg` / `WriteSysReg` / `WriteSysRegImm` as not having side effects. I wonder what is the reason for that.
> > 
> > I'm sure I'm missing something here.
> FRM is incomplete. None of the FP instructions are annotated to with Uses/Defs of FRM.
> 
> I think we found issues with instruction scheduling for VXRM on our internal branch if we didn't have hasSideEffects here. I'll see if I can find the discussion and if there was a test case added. I suspect FRM also needs it, but hasn't been tested as much.
Ok I reviewed the discussion. I think what happens is InstrEmitter.cpp sets the physical register def as Dead because SelectionDAG doesn't see the VXRM instruction glued to any instructions. This allowed MachineCSE to remove or CSE something in a way it shouldn't have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113439