[PATCH] D54719: [Intrinsic] Signed Fixed Point Multiplication Intrinsic

Leonard Chan via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 28 17:47:44 PST 2018


leonardchan marked an inline comment as done.
leonardchan added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2543
+          DAG.getNode(ISD::MUL, dl, VT, LHS, RHS).getNode(), Lo, Hi, NVT, DAG,
+          TargetLowering::MulExpansionKind::OnlyLegalOrCustom, LL, LH, RL, RH))
+    return;
----------------
efriedma wrote:
> ebevhan wrote:
> > leonardchan wrote:
> > > ebevhan wrote:
> > > > Maybe I'm missing something here, but isn't this just a normal, expanded multiplication?
> > > This should actually be an expansion of SMUL_LOHI since we need the high bits when scaling down the result after multiplication. Updated the code, but one thing I found was that `expandMUL_LOHI` depends on `ADDC` but does not have a check for it. When rerunning the tests, it seems that X86 does not support 32 bit `ADDE` (and `ADDC`).
> > > 
> > > ```
> > > LLVM ERROR: Cannot select: t81: i32,glue = adde t121, t44:1, t80:1
> > > ```
> > > 
> > > so I removed these from the lit tests, but if we want to support expansion of this, I'm not sure if the solution is to add a check into `expandMUL_LOHI` to check `isOperationLegalOrCustom(ISD::ADDC/E, VT)` and if it is not legal, write out the expanded version. I found a comment in `ExpandIntRes_ADDSUB()` saying that there currently does not appear to be a way of generating a value of `MTV::Glue` if we were to expand the result.
> > It's a bit odd that x86 doesn't support ADDC/ADDE. Those nodes are deprecated in favor of ADDCARRY, so expandMUL_LOHI should probably be making expansions with ADDCARRY instead.
> > 
> > @craig.topper probably knows more about this.
> Yes, ADDC/ADDE are deprecated, and only work on targets which specifically implement lowering support, so there are a lot of targets where they simply can't be used (either because the target uses ADDCARRY instead, or because the operation simply doesn't exist on the target).  expandMUL_LOHI is only used in very few places, though, so I guess we haven't hit this issue before.
> 
> There are a few different ways to expand an ADD; see DAGTypeLegalizer::ExpandIntRes_ADDSUB.  Ideally, we should just generate ADDCARRY and let legalization take care of the rest, but I don't think ADDCARRY legalization is currently implemented.  Probably not hard to implement, though.  (Maybe keep the existing ADDC/ADDE code for targets which use them.)
I changed `expandMUL_LOHI` to use `ADDCARRY` if `ADDC` and `ADDE` are not available, and the corresponding expansion in  the `DAGLegalizer`.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54719/new/

https://reviews.llvm.org/D54719





More information about the llvm-commits mailing list