This is an archive of the discontinued LLVM Phabricator instance.

[CFLGraph][NFC] Simplify/reorder switch in visitConstantExpr
ClosedPublic

Authored by xbolva00 on Apr 30 2018, 3:19 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

xbolva00 created this revision.Apr 30 2018, 3:19 AM
xbolva00 updated this revision to Diff 144528.Apr 30 2018, 3:22 AM
xbolva00 updated this revision to Diff 144529.Apr 30 2018, 3:32 AM
xbolva00 updated this revision to Diff 144530.
xbolva00 updated this revision to Diff 144531.
xbolva00 added a reviewer: rjmccall.

I'm not sure why I've been CC'ed here, but most of this patch appears to be an arbitrary re-ordering of the cases.

I don't understand at all why this is better/simpler

xbolva00 added a comment.EditedMay 1 2018, 12:56 PM

I don't understand at all why this is better/simpler

Same blocks for case labels (see eg. Select and ShuffleVector, but there are more changes) are regrouped together now, the code readability is better I think.

xbolva00 retitled this revision from [CFLGraph][NFC] Simplify switch in visitConstantExpr to [CFLGraph][NFC] Simplify/reorder switch in visitConstantExpr.May 1 2018, 1:02 PM

Okay, so the actual code changes here are:

  • combining the redundant Insert{Element,Value} and Extract{Element,Value} cases, which is good,
  • eliminating some unnecessary temporary variables from some of the cases, which is also good, and
  • reordering all the cases, which doesn't seem to be an improvement in any way and makes it harder to recognize the first two.

Why don't you submit a patch with just the first two changes?

xbolva00 updated this revision to Diff 144774.EditedMay 1 2018, 1:31 PM

Why don't you submit a patch with just the first two changes?

Sorry, updated revision uses the original ordering.

Thank you, this makes the change much more obvious. You've still reordered the case starting with BitCast, though.

xbolva00 updated this revision to Diff 144780.May 1 2018, 2:06 PM

Thank you, this makes the change much more obvious. You've still reordered the case starting with BitCast, though.

Missed it, updated to fix it :)

xbolva00 updated this revision to Diff 144783.May 1 2018, 2:08 PM
xbolva00 updated this revision to Diff 144784.
xbolva00 updated this revision to Diff 144785.

Fixed braces

I hate to keep making these picky comments, but I don't know what you're trying to do with the braces now. You're adding braces on some of these cases, you're removing them from others, it's all inconsistent. I think it's almost always best to have braces on cases, unless they're just a single-line return or something; so please at least leave the existing braces where they are, and maybe add braces to cases if they don't have them.

xbolva00 added a comment.EditedMay 1 2018, 2:22 PM

I hate to keep making these picky comments, but I don't know what you're trying to do with the braces now. You're adding braces on some of these cases, you're removing them from others, it's all inconsistent. I think it's almost always best to have braces on cases, unless they're just a single-line return or something; so please at least leave the existing braces where they are, and maybe add braces to cases if they don't have them.

It was inconsistent even before :/

Current code has the pattern like "If two addX in the case then use braces, if just one, no braces" so I was not sure. I added them at first and then I removed them just becauce I thought you would blame me that code doesnt follow original "scheme/pattern".

Sorry, thanks for the advice, I will fix it.

xbolva00 updated this revision to Diff 144789.May 1 2018, 2:29 PM
rjmccall accepted this revision.May 1 2018, 2:33 PM

Thanks, this looks great.

This revision is now accepted and ready to land.May 1 2018, 2:33 PM
This revision was automatically updated to reflect the committed changes.
efriedma added inline comments.May 1 2018, 6:08 PM
llvm/trunk/lib/Analysis/CFLGraph.h
548 ↗(On Diff #144792)

The logic for selects looks suspicious; operand 0 is the condition.

xbolva00 added inline comments.May 1 2018, 6:14 PM
llvm/trunk/lib/Analysis/CFLGraph.h
548 ↗(On Diff #144792)

Yep true. Seems like this is quite an old bug here.

We should use operands 1 and 2 here.

I will post a new patch tomorrow.

xbolva00 added inline comments.May 1 2018, 6:23 PM
llvm/trunk/lib/Analysis/CFLGraph.h
548 ↗(On Diff #144792)

Can you look on it?

https://reviews.llvm.org/D46343

Thanks :)