[PATCH] D80285: [mlir] make the bitwidth of device side index computations configurable

[Tobias Gysi via Phabricator via llvm-commits]

gysit marked 3 inline comments as done.
gysit added inline comments.


================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h:385
+                           /*indexBitwidth=*/kDeriveIndexBitwidthFromDataLayout,
+                           /*useAlignedAlloc=*/false},
                        PatternBenefit benefit = 1);
----------------
mehdi_amini wrote:
> jeanPerier wrote:
> > gysit wrote:
> > > ftynse wrote:
> > > > gysit wrote:
> > > > > ftynse wrote:
> > > > > > gysit wrote:
> > > > > > > mehdi_amini wrote:
> > > > > > > > I am puzzled how is this working?
> > > > > > > > 
> > > > > > > > This default value for this parameter is mapped to a reference member, how isn't it gonna lead to "use-after-free"?
> > > > > > > > 
> > > > > > > You are right this should not work (the lifetime of the default argument is limited to the body of the constructor -- I believe). I will submit a patch to fix this problem.
> > > > > > Hmm, why wouldn't it? The lifetime of the temporary is that of the constructor body. The reference will be used to copy-construct the member struct at the beginning of the constructor implementation, at which point the temporary is guaranteed to be live. Then we will only use the member. It would have been a problem if ConvertToLLVMPattern kept a reference to the temporary.
> > > > > >  It would have been a problem if ConvertToLLVMPattern kept a reference to the temporary.
> > > > > 
> > > > > The options member is unfortunately a const reference. 
> > > > > 
> > > > > 
> > > > My bad, I looked elsewhere. The reference capture semantics should be documented somewhere. Or in a more hacky way, this can accept a non-const reference that would effectively disallow passing in temporaries.
> > > should we just make the options a non-reference member of ConvertToLLVMPattern? At the moment to struct is super small and copying the options should not harm performance. 
> > + 1, this broke our flang builds with some compilers (they randomly emitted C interface). 
> Duplicating the option in every single pattern instance inheriting from ConvertToLLVMPattern seems a bit suboptimal to me.
> + 1, this broke our flang builds with some compilers (they randomly emitted C interface).

Sorry for breaking your build. I reverted the commit which hopefully solves your problem. 






================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h:385
+                           /*indexBitwidth=*/kDeriveIndexBitwidthFromDataLayout,
+                           /*useAlignedAlloc=*/false},
                        PatternBenefit benefit = 1);
----------------
gysit wrote:
> mehdi_amini wrote:
> > jeanPerier wrote:
> > > gysit wrote:
> > > > ftynse wrote:
> > > > > gysit wrote:
> > > > > > ftynse wrote:
> > > > > > > gysit wrote:
> > > > > > > > mehdi_amini wrote:
> > > > > > > > > I am puzzled how is this working?
> > > > > > > > > 
> > > > > > > > > This default value for this parameter is mapped to a reference member, how isn't it gonna lead to "use-after-free"?
> > > > > > > > > 
> > > > > > > > You are right this should not work (the lifetime of the default argument is limited to the body of the constructor -- I believe). I will submit a patch to fix this problem.
> > > > > > > Hmm, why wouldn't it? The lifetime of the temporary is that of the constructor body. The reference will be used to copy-construct the member struct at the beginning of the constructor implementation, at which point the temporary is guaranteed to be live. Then we will only use the member. It would have been a problem if ConvertToLLVMPattern kept a reference to the temporary.
> > > > > > >  It would have been a problem if ConvertToLLVMPattern kept a reference to the temporary.
> > > > > > 
> > > > > > The options member is unfortunately a const reference. 
> > > > > > 
> > > > > > 
> > > > > My bad, I looked elsewhere. The reference capture semantics should be documented somewhere. Or in a more hacky way, this can accept a non-const reference that would effectively disallow passing in temporaries.
> > > > should we just make the options a non-reference member of ConvertToLLVMPattern? At the moment to struct is super small and copying the options should not harm performance. 
> > > + 1, this broke our flang builds with some compilers (they randomly emitted C interface). 
> > Duplicating the option in every single pattern instance inheriting from ConvertToLLVMPattern seems a bit suboptimal to me.
> > + 1, this broke our flang builds with some compilers (they randomly emitted C interface).
> 
> Sorry for breaking your build. I reverted the commit which hopefully solves your problem. 
> 
> 
> 
> 
> Duplicating the option in every single pattern instance inheriting from ConvertToLLVMPattern seems a bit suboptimal to me.


Using a reference or a pointer to the options structure are possible alternatives. Both of them have memory lifetime issues if the referenced memory is freed to early. An alternative could be to pass in a callback that returns an options structure (similar to the one used for the type converter before). This solution has no lifetime issues and the memory footprint should be minimal (a function pointer).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80285