[AArch64] Refactor AArch64NamedImmMapper to become dependent on subtarget features.

[vsukharev]

Summary

In order to introduce v8.1a-specific entities, Mappers should be aware of SubtargetFeatures available.
This patch introduces refactoring, that will then allow to easily introduce:
- v8.1-specific "pan" PState for PStateMapper (PAN extension)
- v8.1-specific sysregs for SysRegMapper (LOR,VHE extensions)

[jmolloy]

Hi Vladimir,

Thanks for doing this. I think there's nothing massively contentious in your patch, but it's difficult to review because it's a merge of three different patches, as far as I can see:

1. Adding SubTargetFeatureBits everywhere.
2. Changing the AsmParser to take a string for barriers etc (this seems orthogonal to me)
3. Re-bikeshedding (Pair->Mapper, if/else -> ?:)

So please separate it out into three patches, each of which can be trivially reviewed.

Cheers,

James

lib/Target/AArch64/Utils/AArch64BaseInfo.h

305

Why "SubTargetFeature == 0"? This seems strange. I would have expected just:

return SubTargetFeature & FeatureBits;

[vsukharev]

Hi James,
thank you for comments. I will update the revision with

2. Changing the AsmParser to take a string for barriers etc

and then provide two more patches for

1. Adding SubTargetFeatureBits everywhere.
2. Re-bikeshedding (Pair->Mapper, if/else -> ?:)

Thanks, Vladimir

lib/Target/AArch64/Utils/AArch64BaseInfo.h

305

It is intended way to use for vast majority of Mappers, that don't depend on any subtargetfeature , and are present permanently. For those, SubTargetFeature field will be as default, i.e. 0. and no FeatureBits equal to 0. This makes all current mappers to remain correct, like
DAIFClr : {"daifclr", DAIFClr}
will mean
struct Mapping {
const char *Name = "daifclr";
uint32_t Value = DAIFClr;
uint64_t SubTargetFeature = 0;
};
and this hasFeature will always return true, no matter of subtargetfeatures

[vsukharev]

Patch has been separated by four parts:

• http://reviews.llvm.org/D8579 [AArch64] Move initializations of AArch64NamedImmMapper out of void AArch64Operand::print(...)
• http://reviews.llvm.org/D8582 [AArch64] Rename Pairs to Mappings in AArch64NamedImmMapper
• (this revision) introduction of FeatureBits to AArch64NamedImmMapper::Mapping
• http://reviews.llvm.org/D8584 [AArch64] Handle Cyclone-specific register in common way

[echristo]

I'm going to ask you to hold on for a second on these. I'm not such a huge
fan of having asm parsers be subtarget dependent and would like to take a
look at it.

-eric

[vsukharev]

Eric, thank you for taking care.
Considering this, would you also take a look on chain of patches that are intended to use this subtarget dependency ?
http://reviews.llvm.org/D8498
http://reviews.llvm.org/D8499
http://reviews.llvm.org/D8500
Thanks, Vladimir

[vsukharev]

Eric, AsmParser is already dependent on subtargetfeatures since http://reviews.llvm.org/rL207742 .
The approach has just been extended from SysRegMapper (plus its children MSRMapper, MRSMapper) to other mappers.
Would you comment on your doubts?

[echristo]

Why are you effectively reimplementing the feature sets for this class?

-eric

[echristo]

A better question might be:

Why are all of these classes copying the feature sets and using them with
their own API?

-eric

[vsukharev]

As per echristo request, subtarget features are no longer stored into Mappers.

New revision is based on http://reviews.llvm.org/D8655

[vsukharev]

Eric, (or Tim?)
would you please review my last version? This had blocked the approved chain to commit
http://reviews.llvm.org/D8584 [AArch64] Handle Cyclone-specific register in common way
http://reviews.llvm.org/D8498 [AArch64] Add v8.1a "Privileged Access Never" extension
http://reviews.llvm.org/D8500 [AArch64] Add v8.1a "Virtualization Host Extensions"
http://reviews.llvm.org/D8499 [AArch64] Add v8.1a "Limited Ordering Regions" extension

[vsukharev]

ping...

[jmolloy]

Hi Vlad,

Looks like a mostly mechanical change. I don't like one aspect of it, but if you change that you can commit.

Cheers,

James

lib/Target/AArch64/Utils/AArch64BaseInfo.h

285

I think this is overly convoluted for such a simple function. The same function name does different things depending on the overload (one checks Name and the other Value). I don't think this is intuitive.

Just simply:

bool isNameEqual(std::string Other, uint64_t FeatureBits=~0ULL) {
  if (AvailableForFeatures && !(AvailableForFeatures & FeatureBits))
    return false;
  return Name == Other;
}

And similarly for Value, would be more clear.

[echristo]

Some comments inline.

-eric

lib/Target/AArch64/Utils/AArch64BaseInfo.h

283

This is a pretty terrible name. What do you mean here?

284

Comments are full sentences.

286

I'm not a fan of this interface name. isEqual taking features and not changing the name isn't very descriptive.

297

If you're taking FeatureBits now this should probably change names to make the interface more clear.

Also (and I realize this was already like this) the functions need a comment as to what they're for.

[vsukharev]

Eric, thank you for your feedback
in fact, I have already committed that as http://reviews.llvm.org/rL235089, with slight re-work, addressing last jmolloy's comment
I suspect a line of text after "Differential Revision:" line has prevented post-commit script to process auto-closing this DiffRevision properly. And I don't know at the time how should I fix that: close Revision and put links to commit and back. (Is it a way to do that at all?)
Anyway, should I address your comments as a separate follow-up patch? If so, would you please comment directly to http://reviews.llvm.org/rL235089, because things are changed a bit.

[echristo]

Please just make the changes I've requested in a follow-up commit. I don't
see the need to reply on another commit.

[Eugene.Zelenko]

Committed in rL235089.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

 
     bool Valid;
     auto Mapper = AArch64PRFM::PRFMMapper();
-    StringRef Name = Mapper.toString(MCE->getValue(), Valid);
+    StringRef Name = 
+        Mapper.toString(MCE->getValue(), STI.getFeatureBits(), Valid);
     Operands.push_back(AArch64Operand::CreatePrefetch(prfop, Name,
                                                       S, getContext()));
     return MatchOperand_Success;

 
   bool Valid;
   auto Mapper = AArch64PRFM::PRFMMapper();
-  unsigned prfop = Mapper.fromString(Tok.getString(), Valid);
+  unsigned prfop = 
+      Mapper.fromString(Tok.getString(), STI.getFeatureBits(), Valid);
   if (!Valid) {
     TokError("pre-fetch hint expected");
     return MatchOperand_ParseFail;

     }
     bool Valid;
     auto Mapper = AArch64DB::DBarrierMapper();
-    StringRef Name = Mapper.toString(MCE->getValue(), Valid);
+    StringRef Name = 
+        Mapper.toString(MCE->getValue(), STI.getFeatureBits(), Valid);
     Operands.push_back( AArch64Operand::CreateBarrier(MCE->getValue(), Name,
                                                       ExprLoc, getContext()));
     return MatchOperand_Success;

 
   bool Valid;
   auto Mapper = AArch64DB::DBarrierMapper();
-  unsigned Opt = Mapper.fromString(Tok.getString(), Valid);
+  unsigned Opt = 
+      Mapper.fromString(Tok.getString(), STI.getFeatureBits(), Valid);
   if (!Valid) {
     TokError("invalid barrier option name");
     return MatchOperand_ParseFail;

          "register should be -1 if and only if it's unknown");
 
   auto PStateMapper = AArch64PState::PStateMapper();
-  uint32_t PStateField = PStateMapper.fromString(Tok.getString(), IsKnown);
+  uint32_t PStateField = 
+      PStateMapper.fromString(Tok.getString(), STI.getFeatureBits(), IsKnown);
   assert(IsKnown == (PStateField != -1U) &&
          "register should be -1 if and only if it's unknown");
 

lib/Target/AArch64/Disassembler/AArch64Disassembler.cpp

   Inst.addOperand(MCOperand::CreateImm(crm));
 
   bool ValidNamed;
-  (void)AArch64PState::PStateMapper().toString(pstate_field, ValidNamed);
+  const AArch64Disassembler *Dis = 
+      static_cast<const AArch64Disassembler *>(Decoder);
+  (void)AArch64PState::PStateMapper().toString(pstate_field, 
+      Dis->getSubtargetInfo().getFeatureBits(), ValidNamed);
 
   return ValidNamed ? Success : Fail;
 }

lib/Target/AArch64/InstPrinter/AArch64InstPrinter.cpp

                                          raw_ostream &O) {
   unsigned prfop = MI->getOperand(OpNum).getImm();
   bool Valid;
-  StringRef Name = AArch64PRFM::PRFMMapper().toString(prfop, Valid);
+  StringRef Name = 
+      AArch64PRFM::PRFMMapper().toString(prfop, getAvailableFeatures(), Valid);
   if (Valid)
     O << Name;
   else

   bool Valid;
   StringRef Name;
   if (Opcode == AArch64::ISB)
-    Name = AArch64ISB::ISBMapper().toString(Val, Valid);
+    Name = AArch64ISB::ISBMapper().toString(Val, getAvailableFeatures(), 
+                                            Valid);
   else
-    Name = AArch64DB::DBarrierMapper().toString(Val, Valid);
+    Name = AArch64DB::DBarrierMapper().toString(Val, getAvailableFeatures(), 
+                                                Valid);
   if (Valid)
     O << Name;
   else

   unsigned Val = MI->getOperand(OpNo).getImm();
 
   bool Valid;
-  StringRef Name = AArch64PState::PStateMapper().toString(Val, Valid);
+  StringRef Name = 
+      AArch64PState::PStateMapper().toString(Val, getAvailableFeatures(), Valid);
   if (Valid)
     O << StringRef(Name.str()).upper();
   else

lib/Target/AArch64/Utils/AArch64BaseInfo.h

   struct Mapping {
     const char *Name;
     uint32_t Value;
+    uint64_t AvailableForFeatures;
+    // empty AvailableForFeatures means "always-on"
+    template <typename T> bool isEqual(T Other) const;
+    template <typename T> bool isEqual(T Other, uint64_t FeatureBits) const {
+      if (AvailableForFeatures && !(AvailableForFeatures & FeatureBits))
+        return false;
+      return isEqual<T>(Other);
+    }
   };
 
   template<int N>
   AArch64NamedImmMapper(const Mapping (&Mappings)[N], uint32_t TooBigImm)
     : Mappings(&Mappings[0]), NumMappings(N), TooBigImm(TooBigImm) {}
 
-  StringRef toString(uint32_t Value, bool &Valid) const;
-  uint32_t fromString(StringRef Name, bool &Valid) const;
+  StringRef toString(uint32_t Value, uint64_t FeatureBits, bool &Valid) const;
+  uint32_t fromString(StringRef Name, uint64_t FeatureBits, bool &Valid) const;
 
   /// Many of the instructions allow an alternative assembly form consisting of
   /// a simple immediate. Currently the only valid forms are ranges [0, N) where

   uint32_t TooBigImm;
 };
 
+template <>
+inline bool AArch64NamedImmMapper::Mapping::isEqual(std::string Other) const {
+  return Name == Other; 
+}
+template <>
+inline bool AArch64NamedImmMapper::Mapping::isEqual(uint32_t Other) const {
+  return Value == Other; 
+}
+
 namespace AArch64AT {
   enum ATValues {
     Invalid = -1,    // Op0 Op1  CRn   CRm   Op2

lib/Target/AArch64/Utils/AArch64BaseInfo.cpp

 
 using namespace llvm;
 
-StringRef AArch64NamedImmMapper::toString(uint32_t Value, bool &Valid) const {
+StringRef AArch64NamedImmMapper::toString(uint32_t Value, uint64_t FeatureBits,
+                                          bool &Valid) const {
   for (unsigned i = 0; i < NumMappings; ++i) {
-    if (Mappings[i].Value == Value) {
+    if (Mappings[i].isEqual(Value, FeatureBits)) {
       Valid = true;
       return Mappings[i].Name;
     }

   return StringRef();
 }
 
-uint32_t AArch64NamedImmMapper::fromString(StringRef Name, bool &Valid) const {
+uint32_t AArch64NamedImmMapper::fromString(StringRef Name, uint64_t FeatureBits,
+                                           bool &Valid) const {
   std::string LowerCaseName = Name.lower();
   for (unsigned i = 0; i < NumMappings; ++i) {
-    if (Mappings[i].Name == LowerCaseName) {
+    if (Mappings[i].isEqual(LowerCaseName, FeatureBits)) {
       Valid = true;
       return Mappings[i].Value;
     }

 
   // First search the registers shared by all
   for (unsigned i = 0; i < array_lengthof(SysRegMappings); ++i) {
-    if (SysRegMappings[i].Name == NameLower) {
+    if (SysRegMappings[i].isEqual(NameLower, FeatureBits)) {
       Valid = true;
       return SysRegMappings[i].Value;
     }

   // Now try the instruction-specific registers (either read-only or
   // write-only).
   for (unsigned i = 0; i < NumInstMappings; ++i) {
-    if (InstMappings[i].Name == NameLower) {
+    if (InstMappings[i].isEqual(NameLower, FeatureBits)) {
       Valid = true;
       return InstMappings[i].Value;
     }

 AArch64SysReg::SysRegMapper::toString(uint32_t Bits, uint64_t FeatureBits) const {
   // First search the registers shared by all
   for (unsigned i = 0; i < array_lengthof(SysRegMappings); ++i) {
-    if (SysRegMappings[i].Value == Bits) {
+    if (SysRegMappings[i].isEqual(Bits, FeatureBits)) {
       return SysRegMappings[i].Name;
     }
   }

   // Now try the instruction-specific registers (either read-only or
   // write-only).
   for (unsigned i = 0; i < NumInstMappings; ++i) {
-    if (InstMappings[i].Value == Bits) {
+    if (InstMappings[i].isEqual(Bits, FeatureBits)) {
       return InstMappings[i].Name;
     }
   }