[PATCH] D77874: [AArch32] Armv8.6a Matrix Mul Assembly Parser Support

Tue Apr 21 07:31:22 PDT 2020

LukeGeeson added inline comments.

================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:6336
+      Mnemonic == "vusmmla" || Mnemonic == "vsudot" ||
+      Mnemonic == "vusdot")
     return Mnemonic;
----------------
simon_tatham wrote:
> LukeGeeson wrote:
> > simon_tatham wrote:
> > > Now I look closely, I'm slightly confused about why all of these instructions needed to be added to this if statement. I thought this if statement was supposed to be a list of exceptions to the general rule that a mnemonic ending in a known condition-code string like "eq" or "ne" or "lt" should be broken up into a shorter mnemonic and a conditional suffix. Hence, for example, "wls" is on the list so that it doesn't get misparsed as "w" with condition "ls".
> > > 
> > > Mind you, I'm also slightly confused about what some of the existing mnemonics in this list are here for. But what specific thing fails if you remove this hunk of the diff?
> > Nothing fails when removing this hunk and running tests (check-all, llvm/test, clang/test).
> > 
> > I agree with you in principle, but I suppose here I'm appealing to the age old adage of 'we did it this way here' 
> > 
> > I think we should keep it, and log a separate issue for refactoring this statement in a different issue
> I'm inclined to take the opposite view: if we //don't// add this hunk now, then if we later find something that breaks as a result, we'll know more about why it had to be here!
Very well, I shall remove this hunk

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

https://reviews.llvm.org/D77874