Page MenuHomePhabricator

[Target][ARM] Change VPTMaskValues to the correct encoding
ClosedPublic

Authored by Pierre-vh on Mar 13 2020, 9:10 AM.

Details

Summary

VPTMaskValue was using the "instruction" encoding to represent the masks (= the same encoding as the one used by the instructions in an object file), but it is only used to build MCOperands, so it should use the MCOperand encoding of the masks, which is slightly different.

Note that this has gone unnoticed for a long time because the encodings are identical for most masks (T, TT, TTT, etc.). We only noticed this (with the help of @simon_tatham
) while we were working on upgrading the MVE VPT Block insertion pass because incorrect assembly was generated for things like TTET.

(Adding @samparker to the list of reviewers as he added this enum I believe)

Diff Detail

Event Timeline

Pierre-vh created this revision.Mar 13 2020, 9:10 AM
simon_tatham added inline comments.Mar 17 2020, 8:26 AM
llvm/lib/Target/ARM/Utils/ARMBaseInfo.h
93–94

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?

Pierre-vh marked an inline comment as done.Mar 17 2020, 8:37 AM
Pierre-vh added inline comments.
llvm/lib/Target/ARM/Utils/ARMBaseInfo.h
93–94

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 ?

simon_tatham added inline comments.Mar 18 2020, 3:24 AM
llvm/lib/Target/ARM/Utils/ARMBaseInfo.h
93–94

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.

Pierre-vh marked an inline comment as done.Mar 18 2020, 8:43 AM
Pierre-vh added inline comments.
llvm/lib/Target/ARM/Utils/ARMBaseInfo.h
93–94

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?)

simon_tatham added inline comments.Mar 18 2020, 9:32 AM
llvm/lib/Target/ARM/Utils/ARMBaseInfo.h
93–94

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.

Pierre-vh updated this revision to Diff 251962.Mar 23 2020, 1:56 AM
  • The Enum is now an Enum-Class
  • Added comment in ARMASMParser

Note that this won't compile without the child revision (the update to the MVE VPT Block Insertion Pass) being merged, so I won't commit this until that patch is approved (so I can commit both at the same time)

simon_tatham accepted this revision.Mar 23 2020, 2:27 AM

LGTM, with only one tiny remaining nitpick.

llvm/lib/Target/ARM/Utils/ARMBaseInfo.h
97

Perhaps adjust this comment to say "IT and VPT blocks", since the same values are appropriate to either.

This revision is now accepted and ready to land.Mar 23 2020, 2:27 AM
Pierre-vh updated this revision to Diff 251972.Mar 23 2020, 2:53 AM

Changed "VPT Blocks" to "IT and VPT Blocks" in ARMBaseInfo.h, line 97

Pierre-vh marked an inline comment as done.Mar 23 2020, 2:53 AM
This revision was automatically updated to reflect the committed changes.