[PATCH] D76139: [Target][ARM] Change VPTMaskValues to the correct encoding

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added inline comments.


================
Comment at: llvm/lib/Target/ARM/Utils/ARMBaseInfo.h:103
+  ///   Txyz = xyz1
   enum VPTMaskValue {
+    T = 0b1000,
----------------
Pierre-vh wrote:
> simon_tatham wrote:
> > Since these mask values are now encoded in the common format used by the MCOperand for both `VPT` and `IT` instructions, perhaps they should have a more generic name than `VPTMaskValue`, and live in the `ARM` namespace instead of `ARMVCC`?
> Sure, this is a good idea, but I don't think that I know enough about the backend to find a sensible name for this enum.
> Do you have a suggestion? Perhaps something like `BlockMaskValue` would work ?
I agree it's hard to find a //good// name (both concise and recognizable).

Your suggested name isn't bad, because 'block' and 'mask' are both words used in the ARMARM descriptions of both IT and VPT instructions. I think I'd remove 'Value' (it doesn't add very much description) and replace it with something that hints at conditionalization or predication. How about `enum class CondBlockMask`, in the `llvm::ARM` namespace? Or `PredBlockMask` if you prefer.

Also, since you've copied and pasted my comment from `ARMAsmParser.cpp` to here, you might add a note at the site of the original comment, mentioning that an enum of the mask values is available here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76139