Page MenuHomePhabricator

[InstructionSimplify] Apply sext/trunc after pointer stripping
ClosedPublic

Authored by hliao on Jul 15 2019, 1:47 PM.

Details

Summary
  • As the pointer stripping could trace through addrspacecast now, need to sext/trunc the offset to ensure it has the same width as the pointer after stripping.

Diff Detail

Repository
rL LLVM

Event Timeline

hliao created this revision.Jul 15 2019, 1:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2019, 1:47 PM
hliao added a comment.Jul 15 2019, 1:48 PM

this fixes crashes found on targets where addrspacecast is used.

This is fine with me. Though, I'm unsure if this should not live in the stripAndAccumulateConstantOffsets method.

The problem is that we have two bit-widths now, pointer before and after. Since we have to express offset in one of them, we have a choice.
I think we already guarantee that the offset is representable in the initial bit-width so that was what I choose. Question is, do we want
to change it to be in the resulting bit-width or not. Either way, we should add a comment to the stripAndAccumulateConstantOffsets method
explaining the situation.

hliao added a comment.Jul 15 2019, 3:15 PM

This is fine with me. Though, I'm unsure if this should not live in the stripAndAccumulateConstantOffsets method.

The problem is that we have two bit-widths now, pointer before and after. Since we have to express offset in one of them, we have a choice.
I think we already guarantee that the offset is representable in the initial bit-width so that was what I choose. Question is, do we want
to change it to be in the resulting bit-width or not. Either way, we should add a comment to the stripAndAccumulateConstantOffsets method
explaining the situation.

As I mentioned, without this fix, there are crashes when compiling similar patterns. The issue is that LHS gets the original bit-width as the offset is 0 and RHS gets the one after stripping. Later, as the stripped pointers point the same object, instsimplify will create compare of the offset, that triggers the crash. We have to ensure offset after stripping has the same bit-width.

jdoerfert accepted this revision.Jul 15 2019, 4:19 PM

LGTM, one request and a proposal to fix it slightly differently.

This is fine with me. Though, I'm unsure if this should not live in the stripAndAccumulateConstantOffsets method.

The problem is that we have two bit-widths now, pointer before and after. Since we have to express offset in one of them, we have a choice.
I think we already guarantee that the offset is representable in the initial bit-width so that was what I choose. Question is, do we want
to change it to be in the resulting bit-width or not. Either way, we should add a comment to the stripAndAccumulateConstantOffsets method
explaining the situation.

As I mentioned, without this fix, there are crashes when compiling similar patterns. The issue is that LHS gets the original bit-width as the offset is 0 and RHS gets the one after stripping. Later, as the stripped pointers point the same object, instsimplify will create compare of the offset, that triggers the crash. We have to ensure offset after stripping has the same bit-width.

I understand. That is what I tried to describe above. My question was: Should stripAndAccumulateConstantOffsets return the offset wrt. the bit-width of the input pointer or output pointer? I can see arguments for both. If you think this way is better you can go ahead and commit. If you think the other way around is better, we should move the conversion into stripAndAccumulateConstantOffsets. I personally tend towards the latter.

Either way, we have to add a comment to the stripAndAccumulateConstantOffsets method explaining the choice!

This revision is now accepted and ready to land.Jul 15 2019, 4:19 PM
hliao added a comment.Jul 15 2019, 5:59 PM

LGTM, one request and a proposal to fix it slightly differently.

This is fine with me. Though, I'm unsure if this should not live in the stripAndAccumulateConstantOffsets method.

The problem is that we have two bit-widths now, pointer before and after. Since we have to express offset in one of them, we have a choice.
I think we already guarantee that the offset is representable in the initial bit-width so that was what I choose. Question is, do we want
to change it to be in the resulting bit-width or not. Either way, we should add a comment to the stripAndAccumulateConstantOffsets method
explaining the situation.

As I mentioned, without this fix, there are crashes when compiling similar patterns. The issue is that LHS gets the original bit-width as the offset is 0 and RHS gets the one after stripping. Later, as the stripped pointers point the same object, instsimplify will create compare of the offset, that triggers the crash. We have to ensure offset after stripping has the same bit-width.

I understand. That is what I tried to describe above. My question was: Should stripAndAccumulateConstantOffsets return the offset wrt. the bit-width of the input pointer or output pointer? I can see arguments for both. If you think this way is better you can go ahead and commit. If you think the other way around is better, we should move the conversion into stripAndAccumulateConstantOffsets. I personally tend towards the latter.

Either way, we have to add a comment to the stripAndAccumulateConstantOffsets method explaining the choice!

I prefer the offset to output pointer as that's that API is designed for, strip any constant adjustments. If the final pointer has different bit-width, we'd better prepare that offset matching that pointer's integer width.

This revision was automatically updated to reflect the committed changes.