[PATCH] D73486: [ms] [llvm-ml] Add support for attempted register parsing

[Nico Weber via Phabricator via llvm-commits]

thakis added inline comments.


================
Comment at: llvm/include/llvm/MC/MCParser/MCTargetAsmParser.h:380
+  virtual OperandMatchResultTy
+  tryParseRegister(unsigned &RegNo, SMLoc &StartLoc, SMLoc &EndLoc) = 0;
+
----------------
Can you add a comment about what this does and what it's used for? (Just a summary of the cl desc)


================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:2340
   if (AMDGPU::IsaInfo::hasCodeObjectV3(&getSTI())) {
     if (!updateGprCountSymbols(RegKind, RegNum, RegWidth))
       return nullptr;
----------------
This not having an Error() suggests that maybe updateGprCountSymbols() emits it?

If it doesn't: Can this function now be implemented in terms of tryParseRegister()? (i.e. call that, and if it returns nullptr, emit Error(Tok.getLoc(), "not a valid operand")?)


================
Comment at: llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp:406
+  }
+  return RegNum;
+}
----------------
This looks 100% identical to ParseRegister() in this file; can you implement Parse() in terms of tryParse() here too?

Also, isn't the return type here wrong? I'm guessing you didn't build with experimental targets enabled? (In the GN build, `llvm_targets_to_build = "all"` in your out\gn\args.gn enables all targets; in the cmake build it's -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=AVR (iirc AVR is the only in-tree experimental target))


================
Comment at: llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp:613
 
   return (RegNo == AVR::NoRegister);
 }
----------------
Guess what my comment is here :)


================
Comment at: llvm/lib/Target/BPF/AsmParser/BPFAsmParser.cpp:328
+  if (tryParseRegister(RegNo, StartLoc, EndLoc) != MatchOperand_Success)
+    return Error(StartLoc, "invalid register name");
+  return false;
----------------
Just like this! :)


================
Comment at: llvm/lib/Target/Hexagon/AsmParser/HexagonAsmParser.cpp:1036
 
+OperandMatchResultTy HexagonAsmParser::tryParseRegister(unsigned &RegNo,
+                                                        SMLoc &StartLoc,
----------------
I think it'd be good to be consistent in what function is implemented in terms of which one (i.e. implement Parse in terms of tryParse here too)


================
Comment at: llvm/lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp:708
   }
   return nullptr;
 }
----------------
same comment here


================
Comment at: llvm/lib/Target/MSP430/AsmParser/MSP430AsmParser.cpp:310
 
   return Error(StartLoc, "invalid register name");
 }
----------------
well, you get the idea


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73486