Page MenuHomePhabricator

CodeGenPrepare: preserve inbounds attribute when sinking GEPs
ClosedPublic

Authored by t.p.northover on Mar 5 2019, 10:12 AM.

Details

Reviewers
dmgreen
Summary

This is part of the groundwork for supporting the AArch64 ILP32 ABI.

Targets can potentially emit more efficient code if they know address computations never overflow. For example ILP32 code on AArch64 (which only has 64-bit address computation) can ignore the possibility of overflow with this extra information.

Diff Detail

Repository
rL LLVM

Event Timeline

t.p.northover created this revision.Mar 5 2019, 10:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2019, 10:12 AM
dmgreen added a subscriber: dmgreen.Mar 8 2019, 6:57 AM

This looks generally useful. Please add context

llvm/lib/CodeGen/CodeGenPrepare.cpp
1954

This can perhaps go with the other variables above the enum

1975

If one of the two is inbounds, does that not make the other inbounds? In other words, why would a mismatch in the inbounds flag matter?

2077

I would drop the brackets, but it's only debug output so up to you.

llvm/test/CodeGen/Thumb/addr-modes.ll
37

This isn't really inbounds, is it? But we don't use that info here, (we don't create a gep), so it's alright? It's a little confusing, at least

t.p.northover marked 4 inline comments as done.Mar 11 2019, 6:35 AM

Please add context

Very sorry about that. I used git format-patch and I don't have the -U9999 muscle memory there. Updated patch should have it.

llvm/lib/CodeGen/CodeGenPrepare.cpp
1975

A violation would mean that one of the instructions produces undef and the other produces a known result, so I believe we could combine them by stripping off the inbounds attribute on the merged instruction.

But personally I doubt it's worth the effort. As far as I know, front-ends don't actually mix the two (all C and C++ arithmetic is inbounds or via inttoptr/ptrtoint).

2077

I can replace them by a space, which looks a bit more natural. We don't want it running into the following stuff.

llvm/test/CodeGen/Thumb/addr-modes.ll
37

I think you were right to be suspicious, it could theoretically be turned into a GEP i16, null, %x or something and the tag would be wrong there. I've fixed it in the updated patch.

dmgreen accepted this revision.Mar 12 2019, 5:55 AM

Looks good to me then.

llvm/lib/CodeGen/CodeGenPrepare.cpp
1975

I thought (correct me if I'm wrong) that a violation would produce poison. And because we know the results are used in a load/store, that would be UB. So we can presume that doesn't happen, and having inbounds on one implies inbounds on the other, providing they are otherwise identical.

I did see one example of this making a difference, although it was rather complex, quite a small change (and more often went up, not down). You are probably correct about it not being worth the effort.

This revision is now accepted and ready to land.Mar 12 2019, 5:55 AM
t.p.northover marked an inline comment as done.Mar 12 2019, 7:18 AM
t.p.northover added inline comments.
llvm/lib/CodeGen/CodeGenPrepare.cpp
1975

I thought (correct me if I'm wrong) that a violation would produce poison

Sorry, yes.

And because we know the results are used in a load/store, that would be UB. So we can presume that doesn't happen, and having inbounds on one implies inbounds on the other.

I think we (may) know it's statically used, but there could still be a dynamic check that makes the inbounds poison innocuous but would cause bad things if the wrapping GEP suddenly became inbounds.

dmgreen added inline comments.Mar 12 2019, 7:59 AM
llvm/lib/CodeGen/CodeGenPrepare.cpp
1975

Yeah, I agree. Should rarely come up, and more trouble than it's worth.

t.p.northover closed this revision.Mar 12 2019, 8:26 AM

Thanks for the review. Committed as r355926.