Thanks for all the comments, now addressed all of them @fhahn

@fhahn pinging reviewers :)

In D86906#2279815, @fhahn wrote:

oh, it might be good to slightly adjust the commit message to be a bit more compact and also mention what actual bug is fixed, e.g. something like [AArch64] Avoid paring loads with same result reg.

Further reduced the mir test case.

In D86956#2249916, @fhahn wrote:

Is it possible to add a MIR test for the issue? Also, it looks like the formatting is off a bit. Could you run clang-format-diff on the change?

Revision:

• Fixed code style
• Provided an mir test

Revision:

• Fixed code style
• Reduce the length of the test file

In D84220#2250298, @fhahn wrote:

In D84220#2250221, @congzhe wrote:

rebased and clang format-ed the code, ready to merge

great, thanks! As mentioned earlier, please let us know here if you need someone to commit this change on your behalf, with the name/email you'd like the author field set to.

rebased and clang format-ed the code, ready to merge

In D86956#2249916, @fhahn wrote:

Is it possible to add a MIR test for the issue? Also, it looks like the formatting is off a bit. Could you run clang-format-diff on the change?

@fhahn Thanks for the comments.

@fhahn could you let me know if you have further comments on this revision? Thanks!

In D84220#2208460, @congzhe wrote:

In D84220#2203480, @jdoerfert wrote:

The assertion doesn't work alone. I would still prefer not to skip over non-call sites blindly. @fhahn in addition to blockaddr users, do we expect anything else? We can skip those just fine.

Thank you, now skipped over blockaddr users.

In D84220#2203014, @fhahn wrote:

Looks like the line numbers in my previous comment got messed up. I let them at an earlier version that actually contained the code. It would be good if you could update the patch with a squashed version of all changes.

Thanks for the comment, now squashed all commits into a single one.

In D84220#2203480, @jdoerfert wrote:

The assertion doesn't work alone. I would still prefer not to skip over non-call sites blindly. @fhahn in addition to blockaddr users, do we expect anything else? We can skip those just fine.

Updated and squashed all commits into a single one

Comments addressed:

comments addressed:

In D84220#2185774, @jdoerfert wrote:

FWIW, I don't try to block this. The call site attribute comment is not addressed but other than that this patch "makes sense" assuming we keep the current semantics.

In D84220#2179859, @efriedma wrote:

I'm not sure I understand the motivation. My problem is that if we replace the return values with undef, it should mean there is no use of the returned value of the function live anymore. What am I missing? Maybe provide the motivating example as a test case?

We use "returned" in codegen even if the value has no uses; see https://reviews.llvm.org/D64986 .

In D84220#2180375, @jdoerfert wrote:

In D84220#2180340, @efriedma wrote:

The current definition of "returned" in LangRef is unclear, yes.

In terms of what we actually want, the "must be equal or else instant UB" variation seems more useful than the "must be equal or poison" variation. With "must be equal or poison", basically the only legal optimization is replacing the result with the argument.

I always thought that was the purpose. Could you elaborate what else we want to do?

The only reason I can think of we wouldn't want the instant UB is if it interacts badly with some significant number of optimizations. I'm not coming up with anything it could interact badly with off the top of my head, though, and any transform that did have an issue would also interact badly with noundef.

I agree that it would interact badly with noundef but I was always thinking that would be less of an issue, maybe I'm wrong though.

When it comes to optimizations, I think this shows how problematic the UB semantic is. The codegen interpretation is the problem not IPSCCP (IMHO). Even if you disagree, I doubt this will fix it. Without running it I would suspect https://godbolt.org/z/rG48nz to have at least one returned argument in a ret undef function. @congzhe could you try this?

In D84220#2179764, @lebedev.ri wrote:

In D84220#2179754, @davide wrote:

Florian, can you take a look?

On Tue, Jul 28, 2020 at 10:35 PM Florian Hahn via llvm-commits <llvm-commits@lists.llvm.org> wrote:

On Jul 28, 2020, at 20:32, Congzhe Cao via Phabricator <reviews@reviews.llvm.org> wrote:

congzhe added a comment.

Could you review this patch please? Thank you! @nikic @davide @efriedma

As I mentioned already, I have some comments in phabricator, but cannot submit them because "You do not have permission to edit this object.”

FWIW it is not common to lock down reviews to require Admin permissions.

---

llvm-commits mailing list
llvm-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

Could you review this patch please? Thank you! @nikic @davide @efriedma