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

Thu Mar 31 22:48:52 PDT 2022

skan 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:
> craig.topper wrote:
> > hvdijk 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.
> > > I hope you are okay with me responding out of the order in your message, I think it is easier to read this way.
> > > 
> > > > I'm amazed that we perceive these things so differently.
> > > 
> > > I know, it's a weird and wonderful thing how no two people are alike, but boy can it make communication difficult. :)
> > > 
> > > > [...] 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.
> > > 
> > > I know some people feel that way, but for myself: if I mess up in a commit and you push a fix, I would be happy with that. I would be annoyed that I messed up, but I would be annoyed regardless of whether I am the one who fixes it. That's not the issue for me. What happened here is that I asked in advance if I could have the time to look into the bug and work on a fix, and you said you were okay with that. Then, once I actually started the work, I wanted to finish it too. I would feel the same way if I started work on any bug, regardless of whether it was caused by myself or someone else.
> > > 
> > > > 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?
> > > 
> > > If you have thoughts on what is correct, please do share. It may not come across that way and I apologise for that, but I really do appreciate it. If you do so though, I would still like to actually understand the problem, and come up with a patch to address the problem. When you express your thoughts in a way that bypasses that process by posting the end result instead, the only thing I can do with it is just copy and paste what you wrote. That does not help me understand the problem, and when I then finally do understand the problem, also no longer allows me to come up with a patch for it because that part is done already. That applies to the wording of the comment too.
> > > 
> > > > Please let me know if there is something about my behavior that is not in compliance with the review's conventions.
> > > 
> > > I do not think you did anything that is not in line with LLVM policy, I think these sorts of interpersonal issues are not covered by policy and they probably should not be. Everybody is different, and as long as we have no reason to doubt the good intentions of other contributors, we should try to respond to them in a way that works for them. Sometimes, like in this particular case, we (I do include myself in that) mess up with that, we get a better understanding of what the other contributors are like, and we can change our behaviour and our expectations accordingly for next time.
> > > 
> > > > Of course, we will wait for the response from craig b/c I just made a suggestion here accoding to my understanding of disassmbler.
> > > 
> > > That sounds good, you tracked the original comment to him (thanks for that), so let's give him a chance to comment on it.
> > 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"
> 
> > [...] 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.
> 
> I know some people feel that way, but for myself: if I mess up in a commit and you push a fix, I would be happy with that. I would be annoyed that I messed up, but I would be annoyed regardless of whether I am the one who fixes it. That's not the issue for me. What happened here is that I asked in advance if I could have the time to look into the bug and work on a fix, and you said you were okay with that. Then, once I actually started the work, I wanted to finish it too. I would feel the same way if I started work on any bug, regardless of whether it was caused by myself or someone else.
> 

I didn't propose the patch until you commented "Is that about what you had in mind too?".

I think a simple "No" for this question is unfriendly, it seems like I'm reluctant to share my thoughts. For me, this question sounds like it's saying, if not, please say how you want to implemement it.

> > 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?
> 
> If you have thoughts on what is correct, please do share. It may not come across that way and I apologise for that, but I really do appreciate it. If you do so though, I would still like to actually understand the problem, and come up with a patch to address the problem. When you express your thoughts in a way that bypasses that process by posting the end result instead, the only thing I can do with it is just copy and paste what you wrote. That does not help me understand the problem, and when I then finally do understand the problem, also no longer allows me to come up with a patch for it because that part is done already. That applies to the wording of the comment too.
> 

I don't think my posted comment is end result, that's why I use the word "suggest". You can organize it in your own words and correct inaccuracies. And my intention is to help you understood the problem more quickly.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122540