Page MenuHomePhabricator

InferAddressSpaces: Support memory intrinsics
ClosedPublic

Authored by arsenm on Jan 26 2017, 8:15 PM.

Details

Diff Detail

Event Timeline

arsenm created this revision.Jan 26 2017, 8:15 PM
jlebar added inline comments.Jan 27 2017, 11:13 AM
lib/Transforms/Scalar/InferAddressSpaces.cpp
570

Instead of "handle updating uses", can we just say "Updates uses"?

573

How about we call this updateMemIntrinsicUse, or updateIfIsMemIntrinsic? We can add other stuff to other functions.

574

The nesting here, and particularly the llvm_unreachable after the else below, is pretty hard for me to read.

Could we restructure the control flow as

auto *MI = dyn_cast<MemIntrinsic>(&U);
if (!MI || MI->isVolatile())
  return false;

...

?

584

auto* with dyn_cast, here and elsewhere?

587

Would you mind adding /*foo=*/false here and below? See my now five-year-old screed, http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html

589

Nit: self-to-self

679

It's not just intrinsic users, right? Regular Instructions could have the same problem?

How would you feel if we restructured this to be

Use &U = *I;
++I;
while (I != E && I->getUser() == CurUser) ++I;

if (updateSimplePtrUse(U) || updateMemIntrinsicUse(U))
  continue;

...

This seems to be a consistent level of abstraction, instead of having one function which does an update and the other which just does a check and leaves the update to the caller. And it's also obvious how to extend this code to handle other cases -- just add a new updateFoo function to the if statement.

arsenm added inline comments.Jan 27 2017, 12:53 PM
lib/Transforms/Scalar/InferAddressSpaces.cpp
679

My definition of "SimplePtrUse" was that it has a single pointer operand that can be trivially replaced, so this was specifically avoiding that problem and treating multi users as the complex case. My patches for icmp/select have that problem, and they are handled separately from updateSimplePtrUse

jlebar added inline comments.Jan 27 2017, 1:05 PM
lib/Transforms/Scalar/InferAddressSpaces.cpp
679

I understand, but there's no harm in running the loop even in this case, right?

It just seems to me much simpler conceptually to say:

  • first advance the I iterator as needed.
  • then muck with the instructions.
arsenm updated this revision to Diff 86112.Jan 27 2017, 1:23 PM
arsenm marked 3 inline comments as done.

Address comments

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

Not done?

669

Could we retain some comment here?

This revision is now accepted and ready to land.Jan 27 2017, 6:35 PM
arsenm added inline comments.Jan 27 2017, 7:40 PM
lib/Transforms/Scalar/InferAddressSpaces.cpp
587

Missed a spot, got the other 2

arsenm marked an inline comment as done.Jan 27 2017, 7:44 PM
arsenm added inline comments.
lib/Transforms/Scalar/InferAddressSpaces.cpp
669

I've moved the comment out of skipToNextUser onto the call site

jlebar added inline comments.Jan 29 2017, 5:15 PM
lib/Transforms/Scalar/InferAddressSpaces.cpp
616

s/E/End/? Otherwise, I like the way you've decomposed it here.

arsenm closed this revision.Jan 30 2017, 6:08 PM
arsenm marked 2 inline comments as done.

r293587