This is an archive of the discontinued LLVM Phabricator instance.

[TargetLowering] Make allowsMemoryAccess methode virtual.
ClosedPublic

Authored by ThomasRaoux on Sep 3 2019, 11:25 AM.

Details

Summary

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.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Sep 3 2019, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2019, 11:25 AM
arsenm requested changes to this revision.Sep 3 2019, 11:28 AM
arsenm added a subscriber: arsenm.

This is already a wrapper around the overridable allowsMisalignedMemoryAccesses

This revision now requires changes to proceed.Sep 3 2019, 11:28 AM

Aren't the targets intended to override allowsMisalignedMemoryAccesses? Does that not work for your usage?

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.

(for more context, this is in the context of https://reviews.llvm.org/rL370576 . It's entirely possible I picked a suboptimal target hook, but allowsMemoryAccess seemed correct to me.).

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

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?

arsenm added a comment.Sep 9 2019, 9:43 AM

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'm not happy about it but not too opposed. I'd like someone else's opinion

arsenm added a comment.Sep 9 2019, 9:47 AM

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'm not happy about it but not too opposed. I'd like someone else's opinion

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)

ThomasRaoux edited the summary of this revision. (Show Details)
ThomasRaoux edited the summary of this revision. (Show Details)Sep 9 2019, 6:11 PM

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?

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'm not happy about it but not too opposed. I'd like someone else's opinion

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)

@arsenm , are you okay with this set of changes?

arsenm accepted this revision.Sep 25 2019, 8:39 AM

LGTM

This revision is now accepted and ready to land.Sep 25 2019, 8:39 AM
This revision was automatically updated to reflect the committed changes.