[PATCH] D24076: [ARM] Use __rt_div functions for DIVREM on Windows

[Saleem Abdulrasool via llvm-commits]

compnerd added inline comments.


> ARMISelLowering.cpp:984
> +      Subtarget->isTargetGNUAEABI() || Subtarget->isTargetMuslAEABI() ||
> +      Subtarget->isTargetWindows()) {
>      setOperationAction(ISD::SREM, MVT::i64, Custom);

Wow, this is getting unwieldy.  I wonder if its easier to negate the condition.

> ARMISelLowering.cpp:989-1007
> +    if (Subtarget->isTargetWindows()) {
> +      setLibcallName(RTLIB::SDIVREM_I8,  "__rt_sdiv");
> +      setLibcallName(RTLIB::SDIVREM_I16, "__rt_sdiv");
> +      setLibcallName(RTLIB::SDIVREM_I32, "__rt_sdiv");
> +      setLibcallName(RTLIB::SDIVREM_I64, "__rt_sdiv64");
> +      setLibcallName(RTLIB::UDIVREM_I8,  "__rt_udiv");
> +      setLibcallName(RTLIB::UDIVREM_I16, "__rt_udiv");

Not your fault since you are replicating what is there, but I think that refactoring this a slight bit will be nicer:

  for (const auto &LC : {RTLIB::SDIVREM_I8, RTLIB::SDIVREM_I16, RTLIB::SDIVREM_I32})
    setLibcallName(LC, Subtarget->isTargetWindows() ? "__rt_sdiv" : "__aeabi_idivmod");
  setLibcallName(RTLIB::SDIVREM_I64, Subtarget->isTargetWindows() ? "__rt_sdiv64" : "__aeabi_ldivmod");
  for (const auto &LC : {RTLIB::UDIVREM_I8, RTLIB::UDIVREM_I16, RTLIB::UDIVREM_I32})
    setLibcallName(LC, Subtarget->isTargetWindows() ? "__rt_udiv" : "__aeabi_uidivmod");
  setLibcallName(RTLIB::UDIVREM_I64, Subtarget->isTargetWindows() ? "__rt_udiv64" : " __aeabi_uldivmod");

> ARMISelLowering.cpp:12520
>  
> +  SDLoc dl(N);
> +  if (Subtarget->isTargetWindows()) {

Sink this inside the if condition, as it is unused otherwise.

> ARMISelLowering.cpp:12532
> +    InChain = DAG.getNode(ARMISD::WIN__DBZCHK, dl, MVT::Other, InChain, Denom);
> +  }
> +

How about doing a trivial static function `ExtractDenominator` or even better, create a `WinDBZCheckDenominator` to handle the duplication of the DBZ check.

  static SDValue WinDBZCheckDenominator(SDNode *N, SDValue InChain) {
    SDLoc DL(N);
    Value *Op = N->getOperand(1);
    if (N->getValueType(0) == MVT::32)
      return DAG.getNode(ARMISD::WIN__DBZCHK, DL, MVT::Other, InChain, Op);
    SDValue Lo = DAG.getNode(ISD::EXTRACT_ELEMENT, DL, MVT::i32, Op),
                             DAG.getConstant(0, DL, MVT::i32));
    SDValue Hi = DAG.getNode(ISD::EXTRACT_ELEMENT, DL, MVT::i32, Op),
                             DAG.getConstant(1, DL, MVT::i32));
    return DAG.getNode(ARMISD::WIN__DBZCHK, DL, MVT::Other, InChain,
                       DAG.getNode(ISD::OR, DL, MVT::i32, Lo, Hi));
  }

> dbzchk.ll:185
>  ; CHECK-WIN__DBZCHK-LABEL: j:
> -; CHECK-WIN__DBZCHK: cbz r{{[0-7]}}, .LBB
> -; CHECK-WIN__DBZCHK-NOT: cbz r8, .LBB

Why are you removing the check for the actual dbzchk codegen?

https://reviews.llvm.org/D24076