Page MenuHomePhabricator

Adding Flush support in OpenMPIRBuilder

Authored by kiranchandramohan on Nov 26 2019, 4:47 AM.



This patch adds support for the Flush construct in the OpenMPIRBuilder introduced in D69785. The change is closely based on D69828.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2019, 4:47 AM
kiranchandramohan edited the summary of this revision. (Show Details)Nov 26 2019, 7:55 AM

Minor comments and overall fine approach. Could you update the test so I can accept it? Later I will assume changes like this are made before it is commited.


You can even move the code to the beginning of the function (see my comment below as well).

Nit: I personally prefer conditionals to have braces in all parts or no braces at all. I get itchy when we have a branch with and one without.

81 ↗(On Diff #231043)

I think we settled on CreateXXX as naming scheme to match the IRBuilder for now.

250 ↗(On Diff #231043)

Can you adopt the scheme below (w/o assert for now)

if (!updateToLocation(Loc))

I'm not certain why we support an "invalid" insertion point but the update method also sets the debug information properly.

One more thing, I would not call this NFC, nor WIP for that matter (in the commit message).

@fghanim created a spreadsheet so we can coordinate our porting efforts:

please feel free to add directives there you intent to port.

Addressed review comments.

  1. Adding a test
  2. Change emitFlush to CreateFlush in OpenMPIRBuilder.cpp
  3. Use updateLoc
  4. Fixed formatting
kiranchandramohan retitled this revision from [WIP][NFC] Adding Flush support in OpenMPIRBuilder to Adding Flush support in OpenMPIRBuilder.Nov 29 2019, 4:52 AM
kiranchandramohan marked 3 inline comments as done.
This revision is now accepted and ready to land.Nov 29 2019, 10:54 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2020, 3:00 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript