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

[Eric Astor via Phabricator via llvm-commits]

epastor added a comment.

Thanks for the feedback!



================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:2340
   if (AMDGPU::IsaInfo::hasCodeObjectV3(&getSTI())) {
     if (!updateGprCountSymbols(RegKind, RegNum, RegWidth))
       return nullptr;
----------------
thakis wrote:
> 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")?)
updateGprCountSymbols does emit Errors, so I think we can't avoid that,


================
Comment at: llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp:406
+  }
+  return RegNum;
+}
----------------
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.)


================
Comment at: llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp:613
 
   return (RegNo == AVR::NoRegister);
 }
----------------
thakis wrote:
> Guess what my comment is here :)
Nearly identical, but not quite - again, one unrolls what we lexed, and the other doesn't.


================
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;
----------------
thakis wrote:
> Just like this! :)
Doable here because ParseRegister doesn't lex any tokens if it fails.


================
Comment at: llvm/lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp:708
   }
   return nullptr;
 }
----------------
thakis wrote:
> same comment here
Again, slightly different behavior - tryParse unlexes any tokens eaten, Parse doesn't.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73486