Page MenuHomePhabricator

Support vectors in CastInst::isBitOrNoopPointerCastable
Needs RevisionPublic

Authored by reames on Oct 7 2020, 11:43 AM.

Details

Summary

Add support for vectors to this utility function. As noted in the diffs, this utility is used in several transforms so adding the generic logic gets picked up in several places.

The LangRef wording is rather vague here, but you can see analogous logic in VNCoercion.cpp and the Verifier's rules for inttoptr/ptrtoint.

Note that the isNoopCast function is currently incorrect for vectors. I plan on fixing that in a follow up, and then trying to merge the code paths. (Before this change, isBitOrNoopPointerCastable was merely incomplete, not incorrect.)

Diff Detail

Unit TestsFailed

TimeTest
1,850 mslinux > lldb-api.functionalities/thread/concurrent_events::TestConcurrentNWatchNBreak.py
Script: -- /usr/bin/python3.8 /mnt/disks/ssd0/agent/llvm-project/lldb/test/API/dotest.py -S nm -u CXXFLAGS -u CFLAGS --env ARCHIVER=/usr/bin/ar --env OBJCOPY=/usr/bin/objcopy --env LLVM_LIBS_DIR=/mnt/disks/ssd0/agent/llvm-project/build/./lib --arch x86_64 --build-dir /mnt/disks/ssd0/agent/llvm-project/build/lldb-test-build.noindex -s /mnt/disks/ssd0/agent/llvm-project/build/lldb-test-traces --lldb-module-cache-dir /mnt/disks/ssd0/agent/llvm-project/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /mnt/disks/ssd0/agent/llvm-project/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /mnt/disks/ssd0/agent/llvm-project/build/./bin/lldb --compiler /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --dsymutil /mnt/disks/ssd0/agent/llvm-project/build/./bin/dsymutil --filecheck /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck --yaml2obj /mnt/disks/ssd0/agent/llvm-project/build/./bin/yaml2obj --lldb-libs-dir /mnt/disks/ssd0/agent/llvm-project/build/./lib /mnt/disks/ssd0/agent/llvm-project/lldb/test/API/functionalities/thread/concurrent_events -p TestConcurrentNWatchNBreak.py

Event Timeline

reames created this revision.Oct 7 2020, 11:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2020, 11:43 AM
reames requested review of this revision.Oct 7 2020, 11:43 AM
vtjnash accepted this revision.Oct 7 2020, 11:53 AM

Makes sense to me

This revision is now accepted and ready to land.Oct 7 2020, 11:53 AM
lebedev.ri added inline comments.
llvm/test/Transforms/InstCombine/load.ll
389

This is going in the opposite direction than what we've just recently disscussed/estabilished - we can't/shouldn't introduce int<->ptr casts that weren't in the source code.

lebedev.ri requested changes to this revision.Oct 7 2020, 11:54 AM
This revision now requires changes to proceed.Oct 7 2020, 11:54 AM
reames added inline comments.Oct 7 2020, 12:02 PM
llvm/test/Transforms/InstCombine/load.ll
389

You need to give a lot more context here. This is simple load forwarding - as done by e.g. GVN. If you want to change direction, I think that should be separated from this patch.

Let me try to approach this slightly differently..
In the most original unoptimized IR, were there actually two different loads, one of a pointer and one of an integer?
If not, then the fact that we ended up with two loads is the problem that needs solving.
Does D88979 help?
If not, would it please be possible to see some kind of an end-to-end test showing how that still happens?

llvm/test/Transforms/InstCombine/load.ll
389

Right. It's D88860 for documentation change, and D88842 / D88789 / D88788 for some lengthy discussions.

Roman,

I think you're bringing in a concern to this review which does not belong here. Simple load forwarding is a transformation we implement in multiple locations (GVN, EarlyCSE, InstCombine). This patch doesn't even change how we handle inttoptrs. Existing code today will forward an integer load to a pointer load and insert an inttoptr. All this patch does is be consistent about handling the same cases for vectors. I believe we should separate the concerns, and not commingle them.

To your actual question - which again, I believe is off topic for this review - we could consider extending the load transform to select the "better" of the two types, and potentially insert a cast for the former set of uses instead of the later. If you want to avoid inttoptrs, we could canonicalize this case (consistently across the optimizer, not just here) to loading pointers and casting to ints. I have to admit I don't fully understand the reasoning behind the desire to avoid inttoptr, so I'm not sure if this actually helps you or not.

Roman,

I think you're bringing in a concern to this review which does not belong here.

I personally think it's a question of overall optimization pipeline sanity in light of recent discussions in related patches.
Sure, this isn't broken per-wording, but it is likely mis-directed, and it might be good to evaluate that before making things more entrenched.

Simple load forwarding is a transformation we implement in multiple locations (GVN, EarlyCSE, InstCombine). This patch doesn't even change how we handle inttoptrs. Existing code today will forward an integer load to a pointer load and insert an inttoptr. All this patch does is be consistent about handling the same cases for vectors.

Yes.

I believe we should separate the concerns, and not commingle them.

To your actual question - which again, I believe is off topic for this review - we could consider extending the load transform to select the "better" of the two types, and potentially insert a cast for the former set of uses instead of the later.

No, that was not my question.
My question was:

In the most original unoptimized IR, were there actually two different loads, one of a pointer and one of an integer?

If you want to avoid inttoptrs, we could canonicalize this case (consistently across the optimizer, not just here) to loading pointers and casting to ints. I have to admit I don't fully understand the reasoning behind the desire to avoid inttoptr, so I'm not sure if this actually helps you or not.

Unfortunately, that's the caveat, the whole reason i'm asking this is because i've tried that already in D88842.

llvm/test/Transforms/InstCombine/call.ll
238

Please can you autogenerate the checklines here and precommit?

lebedev.ri added inline comments.Oct 10 2020, 2:22 PM
llvm/lib/IR/Instructions.cpp
3235–3237

I think this is fixed vector specific, so you likely want FixedVectorType.

To your actual question - which again, I believe is off topic for this review - we could consider extending the load transform to select the "better" of the two types, and potentially insert a cast for the former set of uses instead of the later.

No, that was not my question.
My question was:

In the most original unoptimized IR, were there actually two different loads, one of a pointer and one of an integer?

Ah, we're talking past each other. There is no "unoptimized IR" that I'm working from. I found this while doing an audit of code for consistent handling of non-integral pointers, nothing more. Given that, I can not answer the question you asked.

Again, I believe this entire topic of irrelevant for this particular review. If you want to discuss further, I'm happy to jump on a call next week and brainstorm, but continuing the conversation on the broader direction you want to move in here is not helpful.

lebedev.ri resigned from this revision.Oct 10 2020, 11:20 PM

To your actual question - which again, I believe is off topic for this review - we could consider extending the load transform to select the "better" of the two types, and potentially insert a cast for the former set of uses instead of the later.

No, that was not my question.
My question was:

In the most original unoptimized IR, were there actually two different loads, one of a pointer and one of an integer?

Ah, we're talking past each other. There is no "unoptimized IR" that I'm working from. I found this while doing an audit of code for consistent handling of non-integral pointers, nothing more. Given that, I can not answer the question you asked.

How is it "again" if this is the first time you're stating this?
Apologies if i'm being blind and it was said before.

So IOW this isn't actually *known* to happen in reality,
it's a missed opportunity in the existing fold that was found
solely by looking at the folds, not IR.

Please do actually state that in the patches description,
and please do consider saying things like that beforehand
in next patches..

With that in light i guess this should be fine.

Again, I believe this entire topic of irrelevant for this particular review. If you want to discuss further, I'm happy to jump on a call next week and brainstorm, but continuing the conversation on the broader direction you want to move in here is not helpful.

This revision is now accepted and ready to land.Oct 10 2020, 11:20 PM
nlopes added inline comments.Oct 11 2020, 7:46 AM
llvm/test/Transforms/InstCombine/load.ll
389

@reames this is not "simple" load forwarding. This is doing type punning.
For this transformation to be correct the alias analysis algorithm would need to take all integer stores as potential escape sites. And it doesn't.
We have two options here: either we change the alias analysis algorithm to take non-ptr memory operations into account and thus make it way more conservative, or we disallow implicit int<->ptr casts done through memory operations (such that AA doesn't need to care about non-ptr stores).
Since int<->ptr casts are not that frequent, I think it's better to go with the latter option and keep alias analysis as aggressive as we can for the common case.

If we agree on the statement above, this means this patch is not correct. Yes, LLVM is broken in other places, but at least let's not make it more broken that what it already is. I appreciate the effort to make the vector ops equivalent to the scalar ops, but right now it's not useful to do this as we need to fix the scalar ops first.

lebedev.ri added inline comments.
llvm/test/Transforms/InstCombine/load.ll
389

Since int<->ptr casts are not that frequent, I think it's better to go with the latter option and keep alias analysis as aggressive as we can for the common case.

I'm actually tracking towards that with D88789 and now D88979.

lebedev.ri requested changes to this revision.Oct 11 2020, 10:32 AM

I'm just gonna re-block this, since this is likely pretty much the code i will be removing in follow-ups.
Feel free to ignore this though, i won't do tug-of-revert.

This revision now requires changes to proceed.Oct 11 2020, 10:32 AM