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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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
----------------
craig.topper wrote:
> skan wrote:
> > hvdijk wrote:
> > > 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.
> > I'm confused by your comment, are you trying to say that I shouldn't point out what might be the correct implementation in the review, but only what's wrong?
> > 
> > 
> > > I already expressed my frustration earlier already on taking over something that someone else is already working on and doing it yourself.
> > > 
> > 
> > To be clear, if you read the comments under D103427 carefully, (https://reviews.llvm.org/D103427#inline-1170883) I could have raised patch D122537 earlier. The reason I didn't do this is because I understand that every author wants to fix their own bugs.
> > 
> > You asked for my minds in comments https://reviews.llvm.org/D103427#inline-1171752 first. And then I proposed the patch D122537 to illustrate my thoughts.
> > Finally that patch was not updated and was abandoned b/c it was duplicated w/ this one for respect.
> > 
> > Of course, we will wait for the response from craig b/c I just made a suggestion here accoding to my understanding of disassmbler.
> > 
> > I'm amazed that we perceive these things so differently. 
> > 
> > Please let me know if there is something about my behavior that is not in compliance with the review's conventions.
> At the time that comment was written the there was no AdSize64 on the MASKMOVDQU64 or VMASKMOVDQU64 instructions. The only differences between them that the tablegen code would look at was the Not64BitMode and In64BitMode predicates. There were no IC_64BIT_VEX* ENUM_ENTRY in X86DisassemblerDecoderCommon.h. So there was no way for the tables to differentiate them.
I think the missing IC_64BIT_VEX was what I meant by "attribute class"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122540