[PATCH] D35638: A fix for bug33826

[Farhana Aleen via Phabricator via llvm-commits]

Farhana added inline comments.


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:106
+  if (isa<LoadInst>(Inst)) {
+    if (DL.getTypeSizeInBits(ShuffleVecTy) != 256)
+      return false;
----------------
DavidKreitzer wrote:
> Farhana wrote:
> > Farhana wrote:
> > > DavidKreitzer wrote:
> > > > You could make the ShuffleVecTy check more generic by testing this, which is similar to what you had before adding interleaved store support:
> > > > 
> > > > ```
> > > > if (WideInstSize != DL.getTypeSizeInBits(ShuffleVecType) * Factor)
> > > >   return false;
> > > > ```
> > > > 
> > > > I get your point about the profitability. I think your comment in the test adequately captures that.
> > > I cannot write the way you suggested, it would be incorrect. We are checking the ShuffleVecType, so it can't be multiplied with Factor.
> > > 
> > > For store, it will check against 1024 * 4.
> > > 
> > > 
> > But we can make it semi generic by hardcoding the number of elements.
> > 
> > 
> > I cannot write the way you suggested, it would be incorrect. We are checking the ShuffleVecType, so it can't be multiplied with Factor. For store, it will check against 1024 * 4.
> 
> But the code in question is guarded by isa<LoadInst>(Inst). At any rate, what you have is fine if you change 256 to ShuffleElemSize * SupportedNumElem.
if (WideInstSize != DL.getTypeSizeInBits(ShuffleVecType) * Factor)
  return false;

But the code in question is guarded by isa<LoadInst>(Inst).

>> But ShuffleVecType is type of shuffles[0] which can be different load vs store. So, comparing WideInstSize with DL.getTypeSizeInBits(ShuffleVecType) * Factor will result in incorrect value.

When it's a store, it will compare with with 1024 * 4.

At any rate, what you have is fine if you change 256 to ShuffleElemSize * SupportedNumElem.
>> It will produce incorrect result in the current code since ShuffleElemSize is not guaranteed to be 4 at that point. 

I like the way it is since all checks are confined in one place, make it look clean. But I don't mind to change it since you prefer the other way.




https://reviews.llvm.org/D35638