[PATCH] D64805: [AArch64] Expand bcmp() for small comparisons

[Evandro Menezes via Phabricator via llvm-commits]

evandro added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:625
+  Options.MaxNumLoads = TLI->getMaxExpandSizeMemcmp(OptSize);
+  Options.NumLoadsPerBlock = 2;
+  Options.AllowOverlappingLoads = true;
----------------
SjoerdMeijer wrote:
> evandro wrote:
> > courbet wrote:
> > > brzycki wrote:
> > > > SjoerdMeijer wrote:
> > > > > why 2?
> > > > @evandro selected these based on the x8_64 values set in `llvm/lib/Target/X86/X86TargetTransformInfo.cpp`:
> > > > ```
> > > >   Options.MaxNumLoads = TLI->getMaxExpandSizeMemcmp(OptSize);
> > > >   Options.NumLoadsPerBlock = 2;
> > > >   // ...
> > > >   if (ST->is64Bit()) {
> > > >     Options.LoadSizes.push_back(8);
> > > >   }
> > > >   Options.LoadSizes.push_back(4);
> > > >   Options.LoadSizes.push_back(2);
> > > >   Options.LoadSizes.push_back(1);
> > > > ```
> > > > 
> > > > I do not know why x86_64 selected these values.
> > > This does trades loads for jumps: with this setting, each block will be twice as big (more work per block) but there will be half as many blocks.
> > > 
> > > This was found to be more efficient in general on x86, and might not be true for arm. This will have to be benchmarked. 
> > > 
> > > 
> > As @courbet said, the best figure may be different, but this seemed like a sensible starting point.  Since A64 has more registers, methinks that a higher value might be beneficial, but it depends on how many live registers there are.
> These options look a lot more sensible now as TLI is used, but has this actually been benchmarked?
As I said above, not extensively.  But enough to close the gap that motivated this patch.  As it stands, even without carefully mined values, at least the egregious regression from not inlining `bcmp()` is addressed.


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

https://reviews.llvm.org/D64805