[PATCH] D99575: [ARM] Clarify a comment regarding AArch64. NFC.

Reid Kleckner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 31 13:02:46 PDT 2021


rnk added inline comments.


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMWinCOFFObjectWriter.cpp:30
 public:
   ARMWinCOFFObjectWriter(bool Is64Bit)
     : MCWinCOFFObjectTargetWriter(COFF::IMAGE_FILE_MACHINE_ARMNT) {
----------------
I think you can go further and remove the parameter here and in the factory function createARMWinCOFFObjectWriter. We only ever pass false to it. This just appears to be copy-pasta from the MachO version. The ARMELFObjectWriter doesn't have this constructor parameter.


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMWinCOFFObjectWriter.cpp:32
     : MCWinCOFFObjectTargetWriter(COFF::IMAGE_FILE_MACHINE_ARMNT) {
-    assert(!Is64Bit && "AArch64 support not yet implemented");
+    assert(!Is64Bit && "AArch64 is an entirely different target");
   }
----------------
mstorsjo wrote:
> compnerd wrote:
> > ARM64 is a different target, but the COFF writer can be shared across the two right?  What makes it special?  A comment to that effect would be useful.  I think that we might want to try to push this upwards to avoid the parameter all together if the two paths cannot be merged.
> You mean this class? Almost everything in each of them is different between the two, so I don't really see what benefits there would be to stuffing both into one class with conditionals around everything.
Even if there is overlap between aarch64 and arm, the shared code would have to live in MC, which means it would have to live in MCWinCOFFObjectWriter anyway. I think we can do this simplification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99575



More information about the llvm-commits mailing list