[PATCH] D72569: [PowerPC][Future] Add prefixed instruction paddi to future CPU

Thu Jan 23 07:07:14 PST 2020

Yi-Hong.Lyu accepted this revision.
Yi-Hong.Lyu added inline comments.
This revision is now accepted and ready to land.

================
Comment at: llvm/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp:197
+  if (Imm != 0)
+    return MCDisassembler::Fail;
+  Inst.addOperand(MCOperand::createImm(Imm));
----------------
nemanjai wrote:
> Yi-Hong.Lyu wrote:
> > Two things:
> > 
> > 1. There is no test that execute this line. Tests doesn't have enough coverage in this patch.
> > 2. For `paddi 1, 2, 8589934591, 1`, it is disassemblable but invalid on hardware. I imagine we should use `SoftFail` instead of `Fail` here. Please see https://llvm.org/doxygen/classllvm_1_1MCDisassembler.html#a8eb822283e8f3200ca4b2a1ba0174e6a 
> I am not sure what your intent here is. The instruction as you wrote it is **illegal**. It will cause a `SIGILL` not some kind of undefined behaviour or anything like that.
> What would be the advantage of disassembling object code into invalid instruction forms?
Got it. Please add `MC/Disassembler/PowerPC/future-invalid.txt` in other pull request in this patch, otherwise this line is not tested. Other LGTM.

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

https://reviews.llvm.org/D72569