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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/OutputSections.cpp:242
+  bool NonZeroFiller =
+      llvm::support::endian::read32<endianness::native>(Filler.data()) != 0;
+  if (NonZeroFiller)
----------------
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);
}
```


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D54207