[PATCH] D82876: [Alignment][NFC] Migrate TargetTransformInfo::allowsMisalignedMemoryAccesses to Align

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked an inline comment as done.
gchatelet added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:16150
   if ((Ty == MVT::v4i8 || Ty == MVT::v8i8 || Ty == MVT::v4i16) &&
       Alignment >= VT.getScalarSizeInBits() / 8) {
     if (Fast)
----------------
gchatelet wrote:
> courbet wrote:
> > gchatelet wrote:
> > > courbet wrote:
> > > > When `alignment` was `0`, and `v8i8`, this is not an NFC.
> > > `Ty == MVT::v4i8 || Ty == MVT::v8i8 || Ty == MVT::v4i16` so `Ty` is either 32 or 64 bits (`v4i8` is 32, `v8i8` and `v4i16` are 64)
> > > Since `VT` is a `SimpleVT` => `VT.getScalarSizeInBits()` can only be 32 or 64 as well.
> > > Then `VT.getScalarSizeInBits() / 8` can only be 4 or 8 so it doesn't matter whether Alignment is 0 or 1.
> > > 
> > > Or am I missing something?
> > `isSimple` means native to some processor (as opposed to extended). But e.g. `MVT::v8i8` is both a //simple// and //vector// `EVT`. the scalar type for `MVT::v8i8` is `MVT::i8`, so  `VT.getScalarSizeInBits()==8`, i.e. `VT.getScalarSizeInBits() / 8 == 1`
> Ha I see thx for the explanation and good catch!
> 
> @dmgreen @samparker what do you think?
> I fail to see whether you considered `Alignment` being `0` when writing D65580.
> For context see [this line](https://reviews.llvm.org/D82876#inline-761892) and [this line](https://reviews.llvm.org/D82876#inline-761893).
> 
> As-is, the whole test suite still passes.
A gentle ping @dmgreen @samparker


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82876