[PATCH] D28199: [BypassSlowDivision] Use ValueTracking to simplify run-time checks

[Nikolai Bozhenov via Phabricator via llvm-commits]

n.bozhenov added inline comments.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:157
+  //    equal to Dividend.
+  bool ShortCut = DividendShort && !UseSignedOp;
+
----------------
jlebar wrote:
> "Shortcut" is one word, but also, this control flow is very complicated.
> 
> For example, it's confusing that the first `if (ShortCut)` branch, which is in the "basic block for slow divide operation", actually does something very fast.  One could add comments, but they're going to be pretty complicated.  ("New BB for long divide, which we bypass if possible, see definition of ShortCut...")
> 
> It seems to me we really want to say
> 
>   if (DividendShort && !UseSignedOp) {
>     // Do one thing.
>   } else {
>     // Do something different.
>   }
> 
> Could we refactor the code so we can write it this way?
> It seems to me we really want to say
> 
> ```
> if (DividendShort && !UseSignedOp) {
>   // Do one thing.
> } else {
>   // Do something different.
> }
> ```
>
> Could we refactor the code so we can write it this way?

I fully agree that the control flow gets overly complex here. But I did it this way because all the paths re-use some parts of the code, so to get a simpler control flow one have either to perform significant refactoring of the code or to introduce a lot of code duplication. And I was kind of reluctant to refactor the code.

I played with refactoring approach and uploaded a couple of patches D29896 and D29897. The first patch does refactoring and extracts a number of auxilliary methods from insertFastDiv function, so that in the second patch these methods can be reused along different code paths as necessary. By the way, I had to convert a number of variables into object attributes, because the same values are used across all these auxilliary methods.

Justin, do you think such refactoring approach should be preferred?



https://reviews.llvm.org/D28199