Page MenuHomePhabricator

InferAddressSpaces: Handle icmp
ClosedPublic

Authored by arsenm on Jan 26 2017, 11:06 PM.

Details

Diff Detail

Event Timeline

arsenm created this revision.Jan 26 2017, 11:06 PM
jlebar edited edge metadata.Jan 26 2017, 11:37 PM

Well this one is interesting.

I think I see what you're getting at here; if we can ASC away all uses of a generic pointer except the icmp, then keeping around the generic pointers in icmp is just unnecessary register pressure.

But at least on NVPTX, I think this is only safe if we can transform *both* pointers in the icmp, and moreover only if both pointers are transformed to the same AS.

Well this one is interesting.

I think I see what you're getting at here; if we can ASC away all uses of a generic pointer except the icmp, then keeping around the generic pointers in icmp is just unnecessary register pressure.

But at least on NVPTX, I think this is only safe if we can transform *both* pointers in the icmp, and moreover only if both pointers are transformed to the same AS.

The IR type system requires both be the same type, so this is done here. Also I just noticed I forgot to update the TODO saying to do this

jlebar added inline comments.Jan 26 2017, 11:53 PM
lib/Transforms/Scalar/InferAddressSpaces.cpp
779

I don't see where we check that NewV and OtherNewV have the same AS. Shouldn't it be right here?

Sorry if it's obvious; I'm breaking my rule against reviewing patches late at night, and may be embarrassed in the morning.

arsenm updated this revision to Diff 86026.Jan 27 2017, 12:16 AM
arsenm added inline comments.
lib/Transforms/Scalar/InferAddressSpaces.cpp
779

I was confused about what you were saying, but the check turns out to have been added in the patch I forgot to squash which fixed that bug

Just so I know how to read these tests: AS(4) is the flat AS?

lib/Transforms/Scalar/InferAddressSpaces.cpp
791

Can we have a test for icmp x, undef and icmp undef, x?

794

If KOtherSrc->AS() == NewAS, then isn't this ASC redundant? It seems we should only need it if KOtherSrc is undef, and in that case, can we just substitute a new undef value, instead of ASC'ing the old undef?

Just so I know how to read these tests: AS(4) is the flat AS?

Yes

lib/Transforms/Scalar/InferAddressSpaces.cpp
791

I can add another test for undef, x, but there's no point in trying to handle these in the code since these are canonicalized to constants in the RHS

794

I made this too conservative by adding the new check here, so this is backwards. The addrspace check is redundant with the cast. We need to cast the constants, otherwise it's making an assumption about the bit values of the pointer. For undef I think replacing the type directly would be OK, but the ConstantExpr cast should do the right thing (and I think this will be necessary in the future with freeze anyway)

arsenm updated this revision to Diff 86103.Jan 27 2017, 12:50 PM

Don't check type for constants, just cast them

jlebar accepted this revision.Jan 27 2017, 6:40 PM
jlebar added inline comments.
lib/Transforms/Scalar/InferAddressSpaces.cpp
791

sgtm

808

Since this is now generic, we probably shouldn't say "shared" here.

813

Is this comment now out of date? I'm confused about "instead of currently".

Would also prefer for the comment to start with prose. For example:

// If we can infer that both pointers are in the same addrspace, transform e.g.
//   %cmp = icmp eq float*%p, %q
// into
//   %cmp = icmp eq float addrspace(3)* %new_p, %new_q

Is that all we need to say here?

This revision is now accepted and ready to land.Jan 27 2017, 6:40 PM
arsenm updated this revision to Diff 86232.Jan 29 2017, 3:53 PM

Additional checks are necessary with constant expressions

arsenm closed this revision.Jan 30 2017, 6:28 PM

r293593