[PATCH] D122540: [X86] Fix handling of maskmovdqu in x32 differently

[Harald van Dijk via Phabricator via llvm-commits]

hvdijk added inline comments.


================
Comment at: llvm/lib/Target/X86/X86InstrSSE.td:4000-4001
 let ExeDomain = SSEPackedInt, SchedRW = [SchedWriteVecMoveLS.XMM.MR] in {
-let Uses = [EDI], Predicates = [HasAVX,Not64BitMode] in
+// As there is no attribute class for 64-bit and VEX, VMASKMOVDQU and
+// VMASKMOVDQU64 would have a decode conflict. Prefer VMASKMODDQU64.
+let Uses = [EDI], Predicates = [HasAVX], isAsmParserOnly = 1 in
----------------
skan wrote:
> skan wrote:
> > hvdijk wrote:
> > > skan wrote:
> > > > The comment here is not correct. The disassembler knows the mode 16-bit/32-bit/64-bit when decoding an instruction, and VEX prefix does change the instruction context. I believe the conflict is b/c the only difference between the encodings of VMASKMOVDQU and VMASKMOVDQU64 is a address-size prefix.
> > > The comment that we originally had was
> > > ```
> > >   // Special case since there is no attribute class for 64-bit and VEX
> > >   if (Name == "VMASKMOVDQU64") {
> > >     ShouldBeEmitted = false;
> > >     return;
> > >   }
> > > ```
> > > Are you saying that this old comment was also wrong? If so, I will check in more detail what it should be. If not, I will need a bit more help in understanding how the new comment is different in meaning from the old one.
> > Maybe I misunderstood the old comment here, which was added in llvm-svn: 201299. Hi @craig.topper, could you help decide if this comment needs to be updated.
> > The comment that we originally had was
> > ```
> >   // Special case since there is no attribute class for 64-bit and VEX
> >   if (Name == "VMASKMOVDQU64") {
> >     ShouldBeEmitted = false;
> >     return;
> >   }
> > ```
> > Are you saying that this old comment was also wrong? If so, I will check in more detail what it should be. If not, I will need a bit more help in understanding how the new comment is different in meaning from the old one.
> 
> I suggest the following comments:
> 
> 
> ```
> // VMASKMOVDQU and VMASKMOVDQU64 differ only in the
> // presence of the AdSize prefix. However, the disassembler doesn't
> // care about that difference in the instruction definition; it
> // handles 32-bit vs. 64-bit addressing for itself based purely
> // on the 0x67 prefix and the CPU mode. 
> ```
> 
With respect, I already expressed my frustration earlier already on taking over something that someone else is already working on and doing it yourself. I get that the intent is to be helpful but it goes beyond help, the effect it had before was to tell me "the work you have done was a waste of time, we are better off if you throw that away" and that is the effect it is again having now.

I will wait for the clarification on whether the original comment was correct and whether my version of it accurately reflects the original meaning, and change it if needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122540