[Lldb-commits] [PATCH] D127741: [Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64

[Denis via Phabricator via lldb-commits]

treapster created this revision.
treapster added a reviewer: rafaelauler.
Herald added subscribers: jsji, ayermolo, pengfei, rupprecht, hiraditya, 
kristof.beyls.
Herald added a reviewer: jhenderson.
Herald added a reviewer: MaskRay.
Herald added a reviewer: rafauler.
Herald added a reviewer: Amir.
Herald added a reviewer: maksfb.
Herald added a project: All.
treapster requested review of this revision.
Herald added subscribers: llvm-commits, lldb-commits, yota9, StephenFan.
Herald added projects: LLDB, LLVM.

It think it's reasonable to enable all supported extensions when we are 
disassembling something because we don't want to 
[hardcode](hhtps://reviews.llvm.org/D121999) it in gdb, 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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127741

Files:
  bolt/lib/Core/BinaryContext.cpp
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  llvm/lib/MC/MCSubtargetInfo.cpp
  llvm/tools/llvm-objdump/llvm-objdump.cpp


Index: llvm/tools/llvm-objdump/llvm-objdump.cpp
===
--- llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -1693,6 +1693,10 @@
   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 MRI(
   TheTarget->createMCRegInfo(TripleName));
Index: llvm/lib/MC/MCSubtargetInfo.cpp
===
--- llvm/lib/MC/MCSubtargetInfo.cpp
+++ llvm/lib/MC/MCSubtargetInfo.cpp
@@ -198,6 +198,8 @@
   Help(ProcDesc, ProcFeatures);
 else if (Feature == "+cpuhelp")
   cpuHelp(ProcDesc);
+else if (Feature == "+all")
+  Bits.set();
 else
   ApplyFeatureFlag(Bits, Feature, ProcFeatures);
   }
Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1179,16 +1179,9 @@
   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 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()) {
Index: bolt/lib/Core/BinaryContext.cpp
===
--- bolt/lib/Core/BinaryContext.cpp
+++ bolt/lib/Core/BinaryContext.cpp
@@ -124,8 +124,7 @@
 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,


Index: llvm/tools/llvm-objdump/llvm-objdump.cpp
===
--- llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -1693,6 +1693,10 @@
   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 MRI(
   TheTarget->createMCRegInfo(TripleName));
Index: llvm/lib/MC/MCSubtargetInfo.cpp
===
--- llvm/lib/MC/MCSubtargetInfo.cpp
+++ llvm/lib/MC/MCSubtargetInfo.cpp
@@ -198,6 +198,8 @@
   Help(ProcDesc, ProcFeatures);
 else if (Feature == "+cpuhelp")
   cpuHelp(ProcDesc);
+else if (Feature == "+all")
+  Bits.set();
 else
   ApplyFeatureFlag(Bits, Feature, ProcFeatures);
   }
Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLV

[Lldb-commits] [PATCH] D127741: [Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64

[Denis via Phabricator via lldb-commits]

treapster added a comment.

Well, it broke a lot of tests that very explicitly 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127741

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D127741: [Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64

[Denis via Phabricator via lldb-commits]

treapster added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127741

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D127741: [Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64

[Denis via Phabricator via lldb-commits]

treapster added a comment.

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 ` 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127741

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D127741: [Disassembler] Add +all CPU feature to subtargetInfo and use it for disassembling AArch64

[Denis via Phabricator via lldb-commits]

treapster abandoned this revision.
treapster added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127741

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits