This is an archive of the discontinued LLVM Phabricator instance.

CodeGen: Remove AliasAnalysis from regalloc
ClosedPublic

Authored by arsenm on Jun 25 2022, 6:22 AM.

Details

Summary

This was stored in LiveIntervals, but not actually used for anything
related to LiveIntervals. It was only used in one check for if a load
instruction is rematerializable. I also don't think this was entirely
correct, since it was implicitly assuming constant loads are also
dereferenceable.

Remove this and rely only on the invariant+dereferenceable flags in
the memory operand. Set the flag based on the AA query upfront. This
should have the same net benefit, but has the possible disadvantage of
making this AA query nonlazy.

Preserve the behavior of assuming pointsToConstantMemory implying
dereferenceable for now, but maybe this should be changed.

Diff Detail

Event Timeline

arsenm created this revision.Jun 25 2022, 6:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2022, 6:22 AM
arsenm requested review of this revision.Jun 25 2022, 6:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2022, 6:22 AM
Herald added subscribers: aheejin, wdng. · View Herald Transcript
mtrofin added inline comments.Jun 25 2022, 7:13 AM
llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp
888

I think this isn't needed here.

892

this can be removed, since calculateRegAllocScore now doesn't need the AA

arsenm updated this revision to Diff 440198.Jun 27 2022, 6:29 AM
arsenm marked 2 inline comments as done.

Remove another leftover use

lkail added a subscriber: lkail.Jun 27 2022, 7:07 AM
MatzeB accepted this revision.Jun 27 2022, 10:38 AM

I always found it very unfortunate that we keep IR references around in MIR to do alias analysis queries. This appears to remove a lot of those users (all but the ones in the schedule graph construction?), so I highly welcome the change!

Do you think that there is a risk of target-specific ISel or optimiziation not setting the flags properly in this new scheme? Though even if there is risk, I'd rather push this forward this is too nice a cleanup to block it ;-)

Thanks, LGTM

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1616–1633

Nit: I like initializing things in every branch without a default, so you get warnings if you forget to set the value in one of the branches...

This revision is now accepted and ready to land.Jun 27 2022, 10:38 AM

Maybe we should leave this diff open for a day or two to wait for extra feedback?

Do you think that there is a risk of target-specific ISel or optimiziation not setting the flags properly in this new scheme? Though even if there is risk, I'd rather push this forward this is too nice a cleanup to block it ;-)

Yes, this could potentially regress rematerializing instructions from target load intrinsics. We would have to pass AA through getTgtMemIntrinsic to allow targets to set the flag themselves.

Yes, this could potentially regress rematerializing instructions from target load intrinsics. We would have to pass AA through getTgtMemIntrinsic to allow targets to set the flag themselves.

Ah good point. I think we should at least have an issue filed about that API then.

The general approach here seems fine. I suspect we end up calling pointsToConstantMemory for most memory operands at some point anyway.

In general pointsToConstantMemory doesn't imply the pointer is dereferenceable. But in practice, the cases where that happens are probably pretty obscure (like out-of-bounds references to constant arrays).

I always found it very unfortunate that we keep IR references around in MIR to do alias analysis queries.

I don't really see a good alternative. Assuming we don't just throw away all alias analysis information, I'm not sure how we'd represent it. You could try to explicitly construct alias sets during isel, I guess.

qcolombet accepted this revision.Jun 28 2022, 10:48 AM

I always found it very unfortunate that we keep IR references around in MIR to do alias analysis queries. This appears to remove a lot of those users (all but the ones in the schedule graph construction?), so I highly welcome the change!

+1

The general approach here seems fine. I suspect we end up calling pointsToConstantMemory for most memory operands at some point anyway.

In general pointsToConstantMemory doesn't imply the pointer is dereferenceable. But in practice, the cases where that happens are probably pretty obscure (like out-of-bounds references to constant arrays).

Do you think this is worth fixing in a follow on patch? It seems the API requires two separate queries that mostly do the same thing

In general pointsToConstantMemory doesn't imply the pointer is dereferenceable. But in practice, the cases where that happens are probably pretty obscure (like out-of-bounds references to constant arrays).

Do you think this is worth fixing in a follow on patch? It seems the API requires two separate queries that mostly do the same thing

We probably want the existing semantics of pointsToConstantMemory in most places. In particular, consider a variable array index into a global constant; pointsToConstantMemory, but it's not necessarily dereferenceable.

I think we should just add the call to isDereferenceableAndAlignedPointer. (We could come up with a combined API, but I'm not sure it's worth the extra maintenance work...)