Rename old function to explicitly show that it cares only about alignment. The new allowsMemoryAccess call the function related to alignment by default and can be overridden by target to inform whether the memory access is legal or not.
Aren't the targets intended to override allowsMisalignedMemoryAccesses? Does that not work for your usage?
Unfortunately not. The target in question does not have byte or word stores. The intent is for the target hook implementation to return false based upon VT (a trunc-store doesn't change the alignment of the underlying pointer). I don't myself see a rationale why this function shouldn't be overridable, given that it isn't a no-op wrapper around allowsMisalignedMemoryAccesses.
It seems to me like this is a workaround for the quite inadequate system for defining legality for a load/store. I think allowsMemoryAccess is poorly named; It exists to just check if the type is aligned before calling allowsMisalignedMemoryAccesses. Your problem sounds more like we need a better system for checking if a store/trunc store is legal
The comment mentions "Return true if the target supports a memory access of this type for the given address space and alignment". So I assumed it is responsible for more than checking alignment. Is this not correct?
Your problem sounds more like we need a better system for checking if a store/trunc store is legal
I do sort of agree but this is potentially a deep hole to dive into. In particular the issue here isn't actually trunc-stores, as a trunc-store is equivalent to trunc + AssertZext + store. It's that we're creating a store with a memory bitwidth that isn't supported, and undoing that is really hard.
I'm not sure I completely understand your objection though - You've mentioned how allowsMemoryAccess is intended to be used, but why is it intended in that way? I think I may lack some backcontext.
The comment, signature and name appear to me to be the exact test needed in this case?
My concern is about growing multiple, parallel concepts related to load/store legality and the existing mess of hooks to control these sort of combines and would like to avoid potentially making it worse.
We already have a few too many hooks for controlling this sort of thing, and none them seem really sufficient or well thought out. We have the basic register type legality check, whether the load/store (for a type only) is legal, checks for whether truncstore/extload are legal, and allowsMisalignedMemoryAccesses. Beyond that, we have hooks like isLoadBitCastBeneficial/isStoreBitCastBeneficial, canMergeStoresTo, mergeStoresAfterLegalization, and possibly a few more.
Things would probably be better if we had some variant of allowsMemoryAccess that would totally subsume the existing set of legality checks. On the other hand, I don't really want to spend much time thinking about how to make SelectionDAG better at this point.
On the other hand, I don't really want to spend much time thinking about how to make SelectionDAG better at this point.
I agree, that's kind of my worry with opening this pandora's box. Changing hooks in SDAG and maintaining performance parity is very hard, and we have a longer term solution in Global Isel.
Given that, and given the NFC-ness of this patch, are you happy for it to go in?
I would somewhat prefer we split the current function into something like memoryAccessIsLegalForAlignment that has the current behavior, and this would be a separate function defaulting to calling it (Yes, I know this makes the problem worse)
I added a new function allowsMemoryAccessForAlignment to handle the behavior of the previous allowsMemoryAccess function. I replaced the calls every except for DAGCombine as my understanding is that this place really mean to check that the memory access is legal and won't require legalization to expensive code sequence.
Is that what you had in mind?