[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:
> > 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?)
> 
I think I'd make it an `enum class`, because otherwise, you're putting some very generic names like `T` into a namespace that's very widely used. If you see `var = PredBlockMask::T;` in code, you know where to look for the definition; if you see `var = T;` then you probably look for a variable in the containing function before thinking to look here.

This seems like a fine file to put it in, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76139