[Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64

[treapster]

Summary

It think it's reasonable to enable all supported extensions when we are disassembling something because we don't want to hardcode it in lldb, objdump, bolt and whatever other places use disassembler in llvm. The only reason not to do it I can think of is if there are conflicting instructions with the same encoding in different extensions, but it doesn't seem possible within one archeticture. We probably should use +all on x86 and other platforms? I'm not sure how test for this should look like, because disassembler is used by at least three tools I know of(gdb, objdump, BOLT), so it's not clear where should we put it, and how to make sure that all new instructions are added to it. Waiting for your feedback.

[treapster]

Well, it broke a lot of tests that were checking that instructions are not disassembled without explicitly specified extensions, but i don't quite get the point of such tests. If new instructions were added by default in lldb, why can't we do the same with objdump? Why assert that something is not working and thus prevent adding support for it later?

[DavidSpickett]

conflicting instructions with the same encoding in different extensions, but it doesn't seem possible within one archeticture

I seem to remember some system register names changing between architecture *profiles* not version. @labrinea may know more about that.

Well, it broke a lot of tests that were checking that instructions are not disassembled without explicitly specified extensions, but i don't quite get the point of such tests.

Without looking at them in detail I assume they are checking that the proper extension name is printed when it says "requires bla" and that each instruction is properly tied to the correct feature name. This is for when you are assembling as opposed to disassembling.

I agree that objdump should have some kind of "max" setting, even a default. However there will still need to be a way to test what I referred to.

What might be possible is to write the testing such that it doesn't use llvm-objdump. I haven't seen the structure of the tests perhaps this would mean duplicating tests all over the place.

[DavidSpickett]

Now I think about it, tests could do something like "-mattr=-all" to remove it, right?

[MaskRay]

I like the idea of all for llvm-objdump, but it should be a separate change, with -mattr=-all tests.

[srhines]

We had some internal Google folks hit this exact issue recently. I don't think that the same "default" CPU should be used for debugging tools like llvm-objdump, and that is the crux of the matter here. Perhaps updating the test to specify "generic" as the CPU when passed to llvm-objdump for those tests is the right pattern? Then the default for disassembly should be to have all attributes/features enabled unless the developer specifies on the command line that they want a different target.

[treapster]

In D127741#3583559, @DavidSpickett wrote:

I agree that objdump should have some kind of "max" setting, even a default. However there will still need to be a way to test what I referred to.

As you said, instruction requires bla is part of assembler, not disassembler, so it is not affected by the patch. The problem here is that tests do CHECK-UNKNOWN: 83 a9 91 c0 <unknown> which basically checks that objdump cannot disassemble instruction that was assembled in the very same test. If we change that, we can make +all default attribute for objdump and it will disassemble everything the same way GNU objdump and lldb do.

What might be possible is to write the testing such that it doesn't use llvm-objdump. I haven't seen the structure of the tests perhaps this would mean duplicating tests all over the place.

Why not use objdump? If we only check that instructions get disassembled, it will work fine.

[treapster]

For now i'd like to hear more about possible clashing instruction encodings from @labrinea, and if it's not a concern, i think we can fix the tests if we remove CHECK-UNKNOWN: and only call objdump with CHECK-INST. What do you think?

[DavidSpickett]

Why not use objdump? If we only check that instructions get disassembled, it will work fine.

I just remember some tests using llvm-mc to disassemble and some using objdump. Istr that using llvm-mc for assemble and disassemble usually meant having two test files instead of one. Anyway, not important.

objdump with a generic cpu or with the all feature removed sgtm.

[treapster]

Abandoning because @MaskRay already implemented it in D128029 and D128030. Thanks for your time, @MaskRay!

bolt/lib/Core/BinaryContext.cpp

BinaryContext::createBinaryContext(const ObjectFile *File, bool IsPIC,
   StringRef FeaturesStr = "";
   switch (File->getArch()) {
   case llvm::Triple::x86_64:
     ArchName = "x86-64";
     FeaturesStr = "+nopl";
     break;
   case llvm::Triple::aarch64:
     ArchName = "aarch64";
-    FeaturesStr = "+fp-armv8,+neon,+crypto,+dotprod,+crc,+lse,+ras,+rdm,"
-                  "+fullfp16,+spe,+fuse-aes,+rcpc";
+    FeaturesStr = "+all";
     break;
   default:
     return createStringError(std::errc::not_supported,
                              "BOLT-ERROR: Unrecognized machine in ELF file");
   }
 
   auto TheTriple = std::make_unique<Triple>(File->makeTriple());
   const std::string TripleName = TheTriple->str();

lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

if (arch.IsMIPS()) {
     if (arch_flags & ArchSpec::eMIPSAse_msa)
       features_str += "+msa,";
     if (arch_flags & ArchSpec::eMIPSAse_dsp)
       features_str += "+dsp,";
     if (arch_flags & ArchSpec::eMIPSAse_dspr2)
       features_str += "+dspr2,";
   }
 
-  // If any AArch64 variant, enable latest ISA with all extensions.
+  // If any AArch64 variant, enable all features.
   if (triple.isAArch64()) {
-    features_str += "+v9.3a,";
-    std::vector<llvm::StringRef> features;
-    // Get all possible features
-    llvm::AArch64::getExtensionFeatures(-1, features);
-    features_str += llvm::join(features, ",");
-
-    if (triple.getVendor() == llvm::Triple::Apple)
-      cpu = "apple-latest";
+    features_str += "+all";
   }
 
   if (triple.isRISCV()) {
     uint32_t arch_flags = arch.GetFlags();
     if (arch_flags & ArchSpec::eRISCV_rvc)
       features_str += "+c,";
     if (arch_flags & ArchSpec::eRISCV_rve)
       features_str += "+e,";

llvm/lib/MC/MCSubtargetInfo.cpp

static FeatureBitset getFeatures(StringRef CPU, StringRef TuneCPU, StringRef FS,
 
   // Iterate through each feature
   for (const std::string &Feature : Features.getFeatures()) {
     // Check for help
     if (Feature == "+help")
       Help(ProcDesc, ProcFeatures);
     else if (Feature == "+cpuhelp")
       cpuHelp(ProcDesc);
+    else if (Feature == "+all")
+      Bits.set();
     else
       ApplyFeatureFlag(Bits, Feature, ProcFeatures);
   }
 
   return Bits;
 }
 
 void MCSubtargetInfo::InitMCProcessorInfo(StringRef CPU, StringRef TuneCPU,

llvm/tools/llvm-objdump/llvm-objdump.cpp

 static void disassembleObject(const ObjectFile *Obj, bool InlineRelocs) {
   const Target *TheTarget = getTarget(Obj);
 
   // Package up features to be passed to target/subtarget
   SubtargetFeatures Features = Obj->getFeatures();
   if (!MAttrs.empty())
     for (unsigned I = 0; I != MAttrs.size(); ++I)
       Features.AddFeature(MAttrs[I]);
+  else {
+    if (Obj->getArch() == llvm::Triple::aarch64)
+      Features.AddFeature("+all");
+  }
 
   std::unique_ptr<const MCRegisterInfo> MRI(
       TheTarget->createMCRegInfo(TripleName));
   if (!MRI)
     reportError(Obj->getFileName(),
                 "no register info for target " + TripleName);
 
   // Set up disassembler.