Page MenuHomePhabricator

[CostModel] Use MaybeAlign in getMemoryOpCost
AbandonedPublic

Authored by samparker on Jun 1 2020, 7:16 AM.

Details

Reviewers
gchatelet
Summary

Use MaybeAlign in the generic API, like we do in the backends and in BasicTTI.

Diff Detail

Event Timeline

samparker created this revision.Jun 1 2020, 7:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2020, 7:16 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

Why is it superior to use MaybeAlign instead of Align?
MaybeAlign forces the user to consider the None case, this is more logic to consider.
I fail to see why loosening the interface is desirable. Did I miss something?

Well, firstly I'd just like the interface to be consistent throughout the cost model and, since the IR doesn't force memory operations to have an alignment, I don't see why we'd try to enforce that here.

Well, firstly I'd just like the interface to be consistent throughout the cost model and, since the IR doesn't force memory operations to have an alignment, I don't see why we'd try to enforce that here.

We're moving to a world where we want the alignment to be explicit at the IR level for loads and stores (see D77454 and D79900).
So for consistency - which I agree is important - we should go in the opposite direction and straighten the type system with Align wherever possible.
I'm not super familiar with the code TBH, are there any memory operations handled by getMemoryOpCost that are neither load nor store?

We're moving to a world where we want the alignment to be explicit at the IR level for loads and stores

Ah, thanks for the update, it sounds like a good direction.

are there any memory operations handled by getMemoryOpCost that are neither load nor store?

As far as I can tell, no... but I also see little enforcement. The masked intrinsics have their own API but I guess there's nothing stopping a backend from using this API to get a baseline for any of it's 'memory' operations. I will update this patch to move to Align instead.

Looking again at the backends, it looks like a very involved task to migrate to Align - particularly because getMaskedMemoryOpCost still uses an unsigned value. So I'd still like to get this patch in for consistency, without having to make large changes to several of the backends (of which I'm not even sure what I'd need to do). As it stands, I assume there's some implicit casting going so these base implementations aren't even receiving an Align object in most cases.

Looking again at the backends, it looks like a very involved task to migrate to Align - particularly because getMaskedMemoryOpCost still uses an unsigned value. So I'd still like to get this patch in for consistency, without having to make large changes to several of the backends (of which I'm not even sure what I'd need to do). As it stands, I assume there's some implicit casting going so these base implementations aren't even receiving an Align object in most cases.

Migrating to Align/MaybeAlign is indeed quite an involved task.
I've been working on it for almost a year now and that's why I opposed this patch ( to me it's like deconstructing what I've been fighting for :) )

Align/MaybeAlign are especially designed so there are no implicit casting. One have to call .value() and test for value availability in case of MaybeAlign.
I haven't had time to convert all the APIs yet but I intend to. In the meantime I'd appreciate if you could leave it as is.

If you feel strongly about it you can still submit this patch but this will be more work on my plate.

I appreciate that this has consumed a lot of your time :) The only reason why this is a problem for me is that these generic layers can't talk to each other at the moment, and I need this so that I can move some code around in D80984. I actually have a sneaking feeling it was me that introduced this API change in the first place and I just missed these ones.

arsenm added a subscriber: arsenm.Jun 4 2020, 8:04 AM

We should remove MaybeAlign entirely. Nothing is ever truly missing the alignment, and Align(1) is always a acceptable degenerate value

We should remove MaybeAlign entirely. Nothing is ever truly missing the alignment, and Align(1) is always a acceptable degenerate value.

Okay, so for my case where TTIImpl can't pass the object to BasicTTI, I could just check whether alignment is assigned, and if not, then just pass Align(1)?

arsenm added a comment.Jun 4 2020, 8:20 AM

We should remove MaybeAlign entirely. Nothing is ever truly missing the alignment, and Align(1) is always a acceptable degenerate value.

Okay, so for my case where TTIImpl can't pass the object to BasicTTI, I could just check whether alignment is assigned, and if not, then just pass Align(1)?

We should remove MaybeAlign entirely. Nothing is ever truly missing the alignment, and Align(1) is always a acceptable degenerate value.

Okay, so for my case where TTIImpl can't pass the object to BasicTTI, I could just check whether alignment is assigned, and if not, then just pass Align(1)?

Yes. The broken API before was that you were supposed to check the ABI type alignment if the alignment wasn't set on the load/store instruction. Those have been fixed, and there should be a shrinking number of contexts where the alignment is "missing"

We should remove MaybeAlign entirely. Nothing is ever truly missing the alignment, and Align(1) is always a acceptable degenerate value.

Okay, so for my case where TTIImpl can't pass the object to BasicTTI, I could just check whether alignment is assigned, and if not, then just pass Align(1)?

Yes, thx @arsenm for pointing this out.

samparker abandoned this revision.Jun 5 2020, 12:58 AM

Thank you both.