Page MenuHomePhabricator

[mlir] Add padding to 1-D Vector in CRunnerUtils.h
ClosedPublic

Authored by nicolasvasilache on Mar 2 2020, 7:39 AM.

Details

Summary

This revision adds padding for 1-D Vector in the common case of x86
execution with a stadard data layout. This supports properly interfacing
codegen with arrays of e.g. vector<9xf32>.

Such vectors are already assumed padded to the next power of 2 by LLVM
codegen with the default x86 data layout:

define void @test_vector_add_1d_2_3(<3 x float>* nocapture readnone %0,
<3 x float>* nocapture readonly %1, i64 %2, i64 %3, i64 %4, <3 x float>*
nocapture readnone %5, <3 x float>* nocapture readonly %6, i64 %7, i64
%8, i64 %9, <3 x float>* nocapture readnone %10, <3 x float>* nocapture
%11, i64 %12, i64 %13, i64 %14) local_unnamed_addr {
  %16 = getelementptr <3 x float>, <3 x float>* %6, i64 1
  %17 = load <3 x float>, <3 x float>* %16, align 16
  %18 = getelementptr <3 x float>, <3 x float>* %1, i64 1
  %19 = load <3 x float>, <3 x float>* %18, align 16
  %20 = fadd <3 x float> %17, %19
  %21 = getelementptr <3 x float>, <3 x float>* %11, i64 1

The pointer addressing a vector<3xf32> is assumed aligned @16.
Similarly, the pointer addressing a vector<65xf32> is assumed aligned
@512.

This revision allows using objects such as vector<3xf32> properly with
the standard x86 data layout used in the JitRunner. Integration testing
is done out of tree, at the moment such testing fails without this
change.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2020, 7:39 AM

Update assertions.

ftynse accepted this revision.Mar 2 2020, 7:59 AM
This revision is now accepted and ready to land.Mar 2 2020, 7:59 AM
This revision was automatically updated to reflect the committed changes.

I'm seeing an error on Windows with this:

##[error]mlir\include\mlir\ExecutionEngine\CRunnerUtils.h(93,0): Error C2672: 'detail::nextPowerOf2': no matching overloaded function found
   378>E:\agent\_work\4\s\mlir\include\mlir/ExecutionEngine/CRunnerUtils.h(93): error C2672: 'detail::nextPowerOf2': no matching overloaded function found [E:\agent\_work\4\b\tools\mlir\test\mlir-cpu-runner\cblas.vcxproj]
         E:\agent\_work\4\s\mlir\include\mlir/ExecutionEngine/CRunnerUtils.h(94): note: see reference to class template instantiation 'detail::Vector1D<T,Dim,false>' being compiled
##[error]mlir\include\mlir\ExecutionEngine\CRunnerUtils.h(93,0): Error C2975: 'N': invalid template argument for 'detail::nextPowerOf2', expected compile-time constant expression
   378>E:\agent\_work\4\s\mlir\include\mlir/ExecutionEngine/CRunnerUtils.h(93): error C2975: 'N': invalid template argument for 'detail::nextPowerOf2', expected compile-time constant expression [E:\agent\_work\4\b\tools\mlir\test\mlir-cpu-runner\cblas.vcxproj]
         E:\agent\_work\4\s\mlir\include\mlir/ExecutionEngine/CRunnerUtils.h(58): note: see declaration of 'N'
##[error]mlir\include\mlir\ExecutionEngine\CRunnerUtils.h(113,0): Error C2672: 'detail::isPowerOf2': no matching overloaded function found
   378>E:\agent\_work\4\s\mlir\include\mlir/ExecutionEngine/CRunnerUtils.h(113): error C2672: 'detail::isPowerOf2': no matching overloaded function found [E:\agent\_work\4\b\tools\mlir\test\mlir-cpu-runner\cblas.vcxproj]
         E:\agent\_work\4\s\mlir\include\mlir/ExecutionEngine/CRunnerUtils.h(113): note: see reference to class template instantiation 'Vector<T,Dim,>' being compiled
##[error]mlir\include\mlir\ExecutionEngine\CRunnerUtils.h(113,0): Error C2975: 'N': invalid template argument for 'detail::isPowerOf2', expected compile-time constant expression
   378>E:\agent\_work\4\s\mlir\include\mlir/ExecutionEngine/CRunnerUtils.h(113): error C2975: 'N': invalid template argument for 'detail::isPowerOf2', expected compile-time constant expression [E:\agent\_work\4\b\tools\mlir\test\mlir-cpu-runner\cblas.vcxproj]
         E:\agent\_work\4\s\mlir\include\mlir/ExecutionEngine/CRunnerUtils.h(43): note: see declaration of 'N'

@stella.stamenova this seems a bit tricky to fix at a distance for me, can you propose a patch that would selectively disable what your particular version of MSVC does not support for now with a TODO once it can be reactivated?

flaub added a subscriber: flaub.Mar 3 2020, 4:16 PM
flaub added inline comments.
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
42

We should be able to replace this chunk with:

constexpr bool isPowerOf2(int N) { return (!(N & (N - 1))); }

constexpr unsigned nextPowerOf2(int N) {
  if (N < 1)
    return 1;
  return isPowerOf2(N) ? N : 2 * nextPowerOf2((N + 1) / 2);
}

This should work on C++11 and also works with MSVC.

flaub added inline comments.Mar 3 2020, 4:20 PM
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
85

With the above change, this can be:

char padding[detail::nextPowerOf2(sizeof(T[Dim])) - sizeof(T[Dim])];

Which I think is both easier to read and works on MSVC. We should probably try to use constexpr functions over template specialization now that we can use C++11.