[PATCH] D120329: [SelectionDAG] Emit calls to __divei4 and friends for division/remainder of large integers

[Matthias Gehre via Phabricator via llvm-commits]

mgehre-amd added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:3921
+           "Unexpected argument type for lowering");
+    SDValue StackPtr = DAG.CreateStackTemporary(ArgVT, 4);
+    Entry.Node = StackPtr;
----------------
LuoYuanke wrote:
> Just be curious. Why the alignment is 4 for all target?
No particular reason. The interface to the __udivei4  functions is specified as `unsigned int*` right now, so I wanted this to be aligned like an `int`.
I guess we should somehow obtain the alignment of an `int` on the target platform? (How?)

Alternatively, we can make __udivei4  take a `uint32_t*`, so we don't need to guess what an `int` is in this target,
and then use `DAG.getDataLayout().getABITypeAlign(Type::getInt32Ty())`?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:3925
+    Type *ArgTy = ArgVT.getTypeForEVT(*DAG.getContext());
+    Entry.Ty = PointerType::get(ArgTy, 0);
+    Entry.IsSExt = false;
----------------
LuoYuanke wrote:
> Not sure if the pointer is "i32 *" or "i129 *".
The pointer here will be i256* (after i129 is expanded to i256).
The `__udivei4` argument is `unsigned int[]` to allow for any bitsize.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:3950
+                    std::move(Args))
+      .setInRegister()
+      .setSExtResult(/*isSigned*/ false)
----------------
LuoYuanke wrote:
> Will the parameter passed in register for i386?
I got this from the code hat does i128 division lowering. Its not really clear to me when to set this flag, and it seems removing it has no effect on the generated assembly.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:3952
+      .setSExtResult(/*isSigned*/ false)
+      .setZExtResult(/*!isSigned*/ true);
+
----------------
LuoYuanke wrote:
> Given the return value is void, why set the S/ZExt?
Good catch, I will remove those.


================
Comment at: llvm/test/CodeGen/X86/udivmodei5.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=x86_64-unknown | FileCheck %s
+
----------------
LuoYuanke wrote:
> Generate case for i386?
Thanks, will do


================
Comment at: llvm/test/CodeGen/X86/udivmodei5.ll:28
+; CHECK-NEXT:    movq {{[0-9]+}}(%rsp), %rax
+; CHECK-NEXT:    movq {{[0-9]+}}(%rsp), %rdx
+; CHECK-NEXT:    addq $104, %rsp
----------------
LuoYuanke wrote:
> Is there any ABI description that shows how to pass i129 parameter or return i129 value?
My observation is that those get passed around as pointers, and my interpretation is that they are handled like big structs. Also we only seem to generated power-of-2 sizes after SelectionDAG, so we are passing a i256 here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120329