[PATCH] D45760: [MIPS] Emit a left-shift instead of a power-of-two multiply for jump-tables

[Simon Dardis via Phabricator via llvm-commits]

sdardis requested changes to this revision.
sdardis added a comment.
This revision now requires changes to proceed.

I think we need to modify LegalizeDAG .cpp here directly to eliminate the multiplication, as the legalizer has legalized the ISD::BR_JT into an address synthesis, load and multiplication. The legalizer then recursively legalizes the multiplication into a mult/mflo pair.

Another option would be to provide a MipsPat to match the case of multiplication by a constant followed by a mflo, and where the constant is a power of 2 replace it with a shift of the variable operand to multiplication. That approach would be private to the MIPS backend.



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3692-3694
+    // TODO: Should we turn this into a shift if entry size is a power of 2
+    // This would stop MIPS from emitting a dmult+dmlfo for this but could
+    // it possibly break other targets?
----------------
If you look at DAGCombine.cpp:2667, you'll see the folding of

```
fold (mul x, (1 << c)) -> x << c
```

so, if the jump table entry size is a power of two, Index can become a left shift of the log2 of size of the jump table entry, otherwise the multiplication is ok.


================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:1230-1242
+  // Turn constant power-of-two multiplications that only need lo into a shift:
+  // FIXME: this is probably the wrong place to do this transformation but it is
+  // needed to avoid emitting a li/dmult/dmflo instead of a single shift with an
+  // immediate operand during the lowering of jump tables
+  if ((NewOpc == MipsISD::Mult || NewOpc == MipsISD::Multu) && HasLo &&
+      !HasHi) {
+    if (ConstantSDNode *CImm = dyn_cast<ConstantSDNode>(Op.getOperand(1))) {
----------------
This isn't particularly great as it misses the case where we have a MIPSR6 target, as we don't use this function to perform lowering for that target.


================
Comment at: test/CodeGen/Mips/jump-table-mul.ll:3
+; We used to generate a mul+mflo sequence instead of shifting by 2/3 to get the jump table address
+; RUN: llc %s -O2 -mtriple=mips64-unknown-freebsd -target-abi n64 -relocation-model=pic -o /dev/null -debug
+; RUN: llc %s -O2 -mtriple=mips64-unknown-freebsd -target-abi n64 -relocation-model=pic -o - | FileCheck %s
----------------
Spurious RUN: line. If you mean to test the output of -debug, this test also needs: "REQUIRES: asserts", and a FileCheck invocation with a unique prefix.


================
Comment at: test/CodeGen/Mips/jump-table-mul.ll:6
+
+; Function Attrs: noreturn nounwind
+define i64 @test(i64 %arg) local_unnamed_addr #0 {
----------------
You can remove this.


================
Comment at: test/CodeGen/Mips/jump-table-mul.ll:7
+; Function Attrs: noreturn nounwind
+define i64 @test(i64 %arg) local_unnamed_addr #0 {
+; CHECK-LABEL: test:
----------------
local_unnamed_addr #0 is spurious.


================
Comment at: test/CodeGen/Mips/jump-table-mul.ll:72
+
+attributes #0 = { noreturn nounwind }
----------------
this can be dropped.


Repository:
  rL LLVM

https://reviews.llvm.org/D45760