[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/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:2363
+  if (AMDGPU::IsaInfo::hasCodeObjectV3(&getSTI())) {
+    if (!updateGprCountSymbols(RegKind, RegNum, RegWidth))
+      return nullptr;
----------------
But doesn't this then emit an error if we hit this code path?


================
Comment at: llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp:406
+  }
+  return RegNum;
+}
----------------
epastor wrote:
> thakis wrote:
> > 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))
> Not identical - note the UnLex lines 399-400. The key difference is to make sure that if the parse fails, we unroll everything we've done.
> 
> Also, the return type here is correct... I'd missed adding the definition above, instead. (And yes, didn't build with experimental targets enabled.)
Can we then maybe have both call a helper that takes a "Unlex" bool or something? Or have tryParse() return the token the error would have been emitted and then make Parse() set the current token to that in case of error? (The longer the implementation is, the more important this is – ie the X86 code below. But I'd try to be consistent in all parsers if possible.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73486