[PATCH] D28200: [BypassSlowDivision] Do not bypass division of hash-like values
Justin Lebar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Dec 31 10:20:30 PST 2016
jlebar added inline comments.
================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:78
+/// \brief Check if the value looks like a hash.
+///
----------------
s/the value/a value/
================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:88
+/// And even if we are wrong and the value is not a hash, generally, it is still
+/// quite unlikely for such values to fit the shorter type.
+///
----------------
Suggest `s/quite//` and `s/generally//` -- we weaken it with "generally" then strengthen it with "quite", and then what do we mean exactly? :)
================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:88
+/// And even if we are wrong and the value is not a hash, generally, it is still
+/// quite unlikely for such values to fit the shorter type.
+///
----------------
jlebar wrote:
> Suggest `s/quite//` and `s/generally//` -- we weaken it with "generally" then strengthen it with "quite", and then what do we mean exactly? :)
> unlikely that such values will fit into the shorter type
(If you want to use "for", you could use "uncommon for such values to fit into the shorter type", but I think that's less good here.)
================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:105
+ case Instruction::Mul: {
+ // At this point long constants may be moved out of loops (as bitcasts) by
+ // Constant Hoisting. So, an additional check is needed to find out if the
----------------
s/At this point//
================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:106
+ // At this point long constants may be moved out of loops (as bitcasts) by
+ // Constant Hoisting. So, an additional check is needed to find out if the
+ // operand is a constant actually.
----------------
No comma needed after "So" here.
================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:106
+ // At this point long constants may be moved out of loops (as bitcasts) by
+ // Constant Hoisting. So, an additional check is needed to find out if the
+ // operand is a constant actually.
----------------
jlebar wrote:
> No comma needed after "So" here.
I don't actually understand what you mean by the first sentence. Can you try to rephrase wrt what the bitcast has to do with constant hoisting?
================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:107
+ // Constant Hoisting. So, an additional check is needed to find out if the
+ // operand is a constant actually.
+ Value *Op1 = I->getOperand(1);
----------------
actually a constant
================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:125
+ default:
+ break;
+ }
----------------
We don't need this case?
================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:157
+ // values that are unlikely to fit the shorter type (including hashes) and
+ // return VALRNG_LONG for them to disable bypassing.
+ if (isHashLikeValue(Op, shortLen))
----------------
I'd just get rid of the last sentence entirely -- it's clear what's going on.
================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:159
+ if (isHashLikeValue(Op, shortLen))
+ return VALRNG_LONG;
+
----------------
We only want to do this check for the dividend, right? I mean, I guess it doesn't really hurt to do it for the divisor too, but that isn't motivated by hashtables.
================
Comment at: test/CodeGen/X86/bypass-slow-division-64.ll:138
+; CHECK-NOT: divl
+ %c = mul i64 %a, 5229553307 ; the constant doesn't fit 32 bits
+ %res = urem i64 %c, %l
----------------
Can we test with a constant that *does* fit into 32 bits?
https://reviews.llvm.org/D28200
More information about the llvm-commits
mailing list