[PATCH] D103427: [X86] Fix handling of maskmovdqu in X32

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/X86InstrSSE.td:4032-4039
+           AdSize64;
+let Uses = [EDI], Predicates = [UseSSE2,In64BitMode] in
+def MASKMOVDQUX32 : PDI<0xF7, MRMSrcReg, (outs), (ins VR128:$src, VR128:$mask),
+           "addr32 maskmovdqu\t{$mask, $src|$src, $mask}",
+           [(int_x86_sse2_maskmov_dqu VR128:$src, VR128:$mask, EDI)]>,
+           AdSize32 {
+  let AsmVariantName = "NonParsable";
----------------
skan wrote:
> craig.topper wrote:
> > skan wrote:
> > > craig.topper wrote:
> > > > skan wrote:
> > > > > BTW, why do we need add an instruction like `MASKMOVDQUX32` when we already have  `MASKMOVDQU`? They have the same encoding/decoding except a address-size prefix. We can definitly encode  a `0x67` during encoding and and print a "addr32" during decoding according to the mode w/o adding any new intrustion.  If removing `Not64BitMode` of `MASKMOVDQU` may cause a ISEL issue, we should fix it in ISEL.
> > > > The address register is implicit in the instruction. Our addr32 emission is normally based on the register class of the address registers. But we don't have that here. Are you suggesting to hardcode a special case for MASKMOVDQU in the encoder?
> > > Harcode is one of the solution. Another solution is that we can add `X86::IP_HAS_AD_SIZE` to `Flags` of `MCInst` when translating a `MachineInstr` to a `MCInst`, so that a 0x67 will be emitted. `MASKMOVDQU` accesses memory and has implicit use `EDI`,  and we can get such information from a `MachineInstr`.
> > The implicit def will always be RDI because it's part of the tablegen Uses. I think. So SelectionDAG will always create the MachineInstr with it.
> Do you mean MASKMOVDQU use `RDI` rather than `EDI` in MachineInstr?
Oh nevermind, I should have looked more carefully. I didn't realize we had 3 instructions and you want to reduce to 2.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103427