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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh marked an inline comment as done.
Pierre-vh added inline comments.


================
Comment at: llvm/lib/Target/ARM/Utils/ARMBaseInfo.h:103
+  ///   Txyz = xyz1
   enum VPTMaskValue {
+    T = 0b1000,
----------------
simon_tatham wrote:
> 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.
What do you think about this change?

```
namespace ARMVCC {
  enum VPTCodes {
    /* etc */
  };
} // namespace ARMVCC

namespace ARM {
  /// Mask values for VPT Blocks, to be used by MCOperands.
  /// Note that this is different from the "real" encoding used by the
  /// instructions. In this encoding, the lowest set bit indicates the end of
  /// the encoding, and above that, "1" indicates an else, while "0" indicates
  /// a then.
  ///   Tx = x100
  ///   Txy = xy10
  ///   Txyz = xyz1
  enum PredBlockMask {
    /* etc */
  };
} // namespace ARM
```
Also, should `PredBlockMask` stay in that file, or should I move it to another file? (Where?)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76139