This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix handling of maskmovdqu in x32 differently
ClosedPublic

Authored by hvdijk on Mar 26 2022, 10:26 PM.

Details

Summary

This reverts the functional changes of D103427 but keeps its tests, and reimplements the functionality by reusing the existing 32-bit MASKMOVDQU and VMASKMOVDQU instructions as suggested by skan in review. These instructions were previously predicated on Not64BitMode. This reimplementation restores the disassembly of a class of instructions, which will see a test added in followup patch D122449.

These instructions are in 64-bit mode special cased in X86MCInstLower::Lower, because we use flags with one meaning for subtly different things: we have an AdSize32 class which indicates both that the instruction needs a 0x67 prefix and that the text form of the instruction implies a 0x67 prefix. These instructions are special in needing a 0x67 prefix but having a text form that does *not* imply a 0x67 prefix, so we encode this in MCInst as an instruction that has an explicit address size override.

Note that originally VMASKMOVDQU64 was special cased to be excluded from disassembly, as we cannot distinguish between VMASKMOVDQU and VMASKMOVDQU64 and rely on the fact that these are indistinguishable, or close enough to it, at the MCInst level that it does not matter which we use. Because VMASKMOVDQU now receives special casing, even though it does not make a difference in the current implementation, as a precaution VMASKMOVDQU is excluded from disassembly rather than VMASKMOVDQU64.

Diff Detail

Event Timeline

hvdijk created this revision.Mar 26 2022, 10:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2022, 10:26 PM
hvdijk requested review of this revision.Mar 26 2022, 10:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2022, 10:26 PM
hvdijk edited the summary of this revision. (Show Details)Mar 26 2022, 10:26 PM
skan added a subscriber: skan.Mar 28 2022, 6:53 PM

I believe you can come up this patch independently. But I did make some suggestion about potential solutions in D103427 and prove it in D122537. Would you mind adding me as the co-author and reviewer?

hvdijk edited the summary of this revision. (Show Details)Mar 28 2022, 11:14 PM

I believe you can come up this patch independently. But I did make some suggestion about potential solutions in D103427 and prove it in D122537. Would you mind adding me as the co-author and reviewer?

The idea to reuse MASKMOVDQU and VMASKMOVDQU did come from you, the summary should mention that, sure. Edited, I hope that looks okay to you.

And anyone is always welcome to add themself as a reviewer, please feel free to. I would prefer someone else who was completely uninvolved with the implementation to review this as well though, in case there is something we both missed.

hvdijk edited the summary of this revision. (Show Details)Mar 28 2022, 11:28 PM

The summary mentions fixes, but there are no extra tests to show the fix?

The summary mentions fixes, but there are no extra tests to show the fix?

I referenced skan's D122449 in the summary, that change adds the test that fails on main but passes after this change is applied.

skan added a reviewer: skan.Mar 30 2022, 8:00 PM
skan added inline comments.Mar 30 2022, 8:02 PM
llvm/lib/Target/X86/X86InstrSSE.td
4000

Please add comments in the TD file about why you set this instruction isAsmParserOnly.
(see example in X86InstrTSX.td)

hvdijk updated this revision to Diff 419595.Mar 31 2022, 6:10 PM
hvdijk marked an inline comment as done.
hvdijk added inline comments.
llvm/lib/Target/X86/X86InstrSSE.td
4000

Comment added, the wording is based on the comment that was originally there prior to D103427 and the error we get when we do not mark it isAsmParserOnly.

hvdijk marked an inline comment as done.Mar 31 2022, 6:12 PM
skan added inline comments.Mar 31 2022, 6:40 PM
llvm/lib/Target/X86/X86InstrSSE.td
4000–4001

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.

hvdijk added inline comments.Mar 31 2022, 6:45 PM
llvm/lib/Target/X86/X86InstrSSE.td
4000–4001

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.

skan added inline comments.Mar 31 2022, 7:11 PM
llvm/lib/Target/X86/X86InstrSSE.td
4000–4001

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.

skan added inline comments.Mar 31 2022, 7:16 PM
llvm/lib/Target/X86/X86InstrSSE.td
4000–4001

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.
hvdijk added inline comments.Mar 31 2022, 8:20 PM
llvm/lib/Target/X86/X86InstrSSE.td
4000–4001

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.

skan added inline comments.Mar 31 2022, 9:12 PM
llvm/lib/Target/X86/X86InstrSSE.td
4000–4001

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.

craig.topper added inline comments.Mar 31 2022, 10:00 PM
llvm/lib/Target/X86/X86InstrSSE.td
4000–4001

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.

craig.topper added inline comments.Mar 31 2022, 10:02 PM
llvm/lib/Target/X86/X86InstrSSE.td
4000–4001

I think the missing IC_64BIT_VEX was what I meant by "attribute class"

hvdijk added inline comments.Mar 31 2022, 10:19 PM
llvm/lib/Target/X86/X86InstrSSE.td
4000–4001

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.

hvdijk added inline comments.Mar 31 2022, 10:42 PM
llvm/lib/Target/X86/X86InstrSSE.td
4000–4001

Thanks, combining that with what @skan wrote earlier, I think that would mean the comment was correct at the time, but became outdated and never got updated. If so, if we reinstate the comment (even if elsewhere) that is a good time to fix that. I will check to make sure my understanding is correct.

skan added inline comments.Mar 31 2022, 10:48 PM
llvm/lib/Target/X86/X86InstrSSE.td
4000–4001

[...] 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.

skan added a comment.Apr 1 2022, 1:55 AM

Anyway, let's focus on the patch itself and ignore the misunderstandings caused by culture or personality during the communication process. Our purpose is to fix the issue and improve the code quality.

hvdijk updated this revision to Diff 420736.Apr 6 2022, 1:31 AM
hvdijk added inline comments.Apr 6 2022, 1:33 AM
llvm/lib/Target/X86/X86InstrSSE.td
4000–4001

Having now had the time to look into this, I think the comment was right and remained right until this change. (Technically, it could be argued to remain accurate even with this change, but only because it is slightly ambiguous and the required interpretation of the comment to make it correct has changed. That means we should treat it as no longer accurate.)

Originally, VMASKMOVDQU was only defined for 32-bit CPU mode, and VMASKMOVDQU64 was only defined for 64-bit CPU mode. However like @craig.topper wrote there is no separate IC_64BIT_VEX so we would not be able to use this to distinguish between the two instructions for decoding. (In response to "The disassembler knows the mode 16-bit/32-bit/64-bit when decoding an instruction, and VEX prefix does change the instruction context." -- Yes, but the VEX prefix changes the instruction context to IC_VEX unconditionally, dropping the distinction between 32-bit vs 64-bit.)

Now, VMASKMOVDQU is defined for both 32-bit and 64-bit CPU mode, and VMASKMOVDQU64 is defined only for 64-bit CPU mode. This means we cannot use CPU mode to disambiguate anyway. These instructions do differ, as @skan wrote, in address size (32-bit vs 64-bit), which is normally also handled by having different instruction contexts, but in the case of VEX those different instruction contexts are not present either so we still cannot use that to disambiguate for decoding.

I believe the comment is correct and clearer like this, does this look okay?

skan accepted this revision.Apr 6 2022, 1:51 AM

LGTM

This revision is now accepted and ready to land.Apr 6 2022, 1:51 AM

I mentioned that I'd prefer to get another person to review this as well but in the interest of just getting the bug fixed, I'll merge this tomorrow if there is no more feedback.

RKSimon accepted this revision.Apr 12 2022, 1:24 AM

LGTM

This revision was landed with ongoing or failed builds.Apr 12 2022, 10:32 AM
This revision was automatically updated to reflect the committed changes.