[PATCH] D92789: [PPC] Check for PPC64 when emitting 64bit specific VSX nodes when pattern matching built vectors

[Zarko Todorovski via Phabricator via llvm-commits]

ZarkoCA added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:2412
 // [HasVSX]
 // [HasVSX, IsBigEndian]
 // [HasVSX, IsLittleEndian]
----------------
Xiangling_L wrote:
> ZarkoCA wrote:
> > Xiangling_L wrote:
> > > I am wondering why do we leave alone `[HasVSX, IsBigEndian]` and `[HasVSX, HasOnlySwappingMemOps, IsBigEndian]` and `HasVSX, HasP8Vector, NoP9Vector, IsBigEndian]`?
> > That's a good question, I tried disabling these so as to have minimal disruption to existing code. And did it by elimination.  
> > 
> > So, as far as I can see the potentially big problem in the pattern matching is later implementations like `[HasVSX, IsISA3_0, HasDirectMove, IsBigEndian]` were implemented without considering 32bit codegen and I added a check there and moved on to earlier ones. At some point, I started hitting test case failures and found that codegen in those cases was correct. And in those cases I found that there were checks added for 32bit like in this patch: https://reviews.llvm.org/D17711.  
> > 
> > In short, I tried to do the safe thing for 32bit VSX without major disruption to existing test cases and codegen unless I could plainly tell it wasn't correct. Which in those cases I wasn't able to. 
> I see your point here. I am asking because I tried adding `IsPPC64` for `[HasVSX, HasP8Vector, NoP9Vector, IsBigEndian]` and found no regressions. So I feel the current situation is kinda messy by having partial fixes and disablement for 32bit mode. But I understand that you want this patch to be conservative correct.
> 
> I am wondering Is that possible for us to add `IsPPC64` for all predicates where no existing testcases would be failed? But if it would take you too much time, I can live with that for now. Since as we discussed offline that we do have plan to verify them one by one for 32bit mode in the near future.

>  I see your point here. I am asking because I tried adding IsPPC64 for [HasVSX, HasP8Vector, NoP9Vector, IsBigEndian] and found no regressions. So I feel the current situation is kinda messy by having partial fixes and disablement for 32bit mode. But I understand that you want this patch to be conservative correct. 

Ah ok, I likely missed this instance. I will have a look and update the patch. Thanks. 

And yes, the plan is to enable 32Bit VSX over time either by extending the implementation here if possible. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92789