[PATCH] D54207: [ELF] Make TrapInstr and Filler byte arrays. NFC.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM with this change.



================
Comment at: ELF/OutputSections.cpp:242
+  bool NonZeroFiller =
+      llvm::support::endian::read32<endianness::native>(Filler.data()) != 0;
+  if (NonZeroFiller)
----------------
grimar wrote:
> jrtc27 wrote:
> > grimar wrote:
> > > grimar wrote:
> > > > jrtc27 wrote:
> > > > > ruiu wrote:
> > > > > > jrtc27 wrote:
> > > > > > > ruiu wrote:
> > > > > > > > Do you need to use read32 if you only want to know whether a value is zero or not?
> > > > > > > We need to know whether all four elements are zero. We could check each element individually (either in a loop or unrolled), but this seemed nicer, and I'm assuming it'll be optimised to a single 32-bit load on architectures with unaligned loads.
> > > > > > Ah, but this is not a performance critical path at all, so I'd just use read32le.
> > > > > I don't particularly like unnecessarily slowing down the code for big-endian hosts, even if it isn't a critical path. The only advantage I see is that it shortens the code a tiny bit, but that's only because there is no `read32ne` wrapper for this like `read32[bl]e`... So I would advocate for keeping it as-is, but at the end of the day I'll change it if necessary.
> > > > Does not seem you need `llvm::support::endian::` prefix here since you added the `using`?
> > > > 
> > > > (And I would also just use our `write32` from Target.h here.
> > > > ```
> > > > inline void write32(void *P, uint32_t V) {
> > > >   llvm::support::endian::write32(P, V, Config->Endianness);
> > > > }
> > > > ```
> > > > 
> > > > This code is called once for the output section, so it's about .. 10-30 calls in average? 
> > > > Does not seem worth to think about slow down here.)
> > > I meant 'read32' of course:
> > > 
> > > ```
> > > inline uint32_t read32(const void *P) {
> > >   return llvm::support::endian::read32(P, Config->Endianness);
> > > }
> > > ```
> > Well, that then uses the *target* endianness instead, which doesn't seem to be an improvement over just using LE or BE unconditionally (though I guess the common case is target == host, where it wouldn't byte swap on either BE or LE...).
> Improvement here is simplicity. There is no point to use LE or BE and even think about which endianness should be used here. Because for this case it is not important and for a new code reader to see `read32` is much better than to see `llvm::support::endian::read32<endianness::native>`, the first one does not raise questions.
Yeah, `read32` is just more concise than `llvm::support::endian::read32<endianness::native>`, that's why I like that.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D54207