Page MenuHomePhabricator

NewGVN: Handle addrspacecast
ClosedPublic

Authored by arsenm on May 18 2019, 8:23 AM.

Details

Summary

The AllConstant check needs to be moved out of the if/else if chain to
avoid a test regression. The "there is no SimplifyZExt" comment puzzles me,
since there is SimplifyCastInst. Additionally, the Simplify* calls
seem to not see the operand as constant, so this needs to be tried if
the simplify failed.

Diff Detail

Event Timeline

arsenm created this revision.May 18 2019, 8:23 AM

This generally makes sense to me. I left a comment as it would be preferable to split the two conceptual changes if we can test them separately.
I think we also need addrspacecast tests with different address spaces to show they are not accidentally merged.

lib/Transforms/Scalar/NewGVN.cpp
1181

The way I understand this: If you have a BitCast, or maybe even another instruction, for which the simplify failed but AllConstant is true, you won't get constant folding before, correct?

Would it be possible to show the problem even w/o AddrSpaceCast?

fhahn added inline comments.Jun 2 2019, 9:53 PM
lib/Transforms/Scalar/NewGVN.cpp
1171

The code here should pass in E->getOperand(0), instead of CI->getOperand(0): we want to pass in the leader for the operand, not the operand from the original IR. This way, there should be no need to move the AllConstant condition. If the leader of the CastInst is constant, we should already simplify it here.

Please note that there is a subtle underlying issue with the way simplifications on the original IR and the simplified leaders interact, but that needs to be addressed separately and I do not think CastInst should have any special treatment here. If anything, it may make it a bit harder to trigger the issue.

arsenm updated this revision to Diff 202718.Jun 3 2019, 7:17 AM

Pass correct operand, don't move AllConstant

arsenm marked an inline comment as done.Jun 3 2019, 7:20 AM
fhahn added a comment.Jun 3 2019, 8:48 AM

LGTM. Could you also add a test where we need to pass in the leader of the operand? Something like having 2 loads from the same address would probably be enough.

fhahn accepted this revision.Jun 3 2019, 8:48 AM
This revision is now accepted and ready to land.Jun 3 2019, 8:48 AM
arsenm closed this revision.Jun 5 2019, 2:13 PM

r362653