Page MenuHomePhabricator

[mlir] NFC: Rename index_t to index_type
ClosedPublic

Authored by ro on Jan 13 2020, 6:48 AM.

Details

Summary

mlir currently fails to build on Solaris:

/vol/llvm/src/llvm-project/dist/mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp:78:20: error: reference to 'index_t' is ambiguous
  IndexHandle zero(index_t(0)), one(index_t(1));
                   ^
/usr/include/sys/types.h:103:16: note: candidate found by name lookup is 'index_t'
typedef short           index_t;
                        ^
/vol/llvm/src/llvm-project/dist/mlir/include/mlir/EDSC/Builders.h:27:8: note: candidate found by name lookup is 'mlir::edsc::index_t'
struct index_t {
       ^

and many more.

Given that POSIX reserves all identifiers ending in _t 2.2.2 The Name Space, it seems
quite unwise to use such identifiers in user code, even more so without a distinguished
prefix.

The following patch fixes this by adding a mlir_ prefix as is already used in other
cases.

Tested on amd64-pc-solaris2.11 and sparcv9-sun-solaris2.11.

Ok for master?

Btw., it would be helpful if mlir were either listed in the global CODE_OWNERS.TXT or had one of its own.

Diff Detail

Event Timeline

ro created this revision.Jan 13 2020, 6:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2020, 6:48 AM

Hello Rainer,

could we sed this to index_type instead and address the _t suffix once and for all as you suggest in your description of the problem?
I am not attached to _t and I see the downside now.

ro updated this revision to Diff 238256.Jan 15 2020, 7:29 AM
ro retitled this revision from [mlir] NFC: Rename index_t to mlir_index_t to [mlir] NFC: Rename index_t to index_type.

Rename index_t to index_type instead.

ro added a comment.Jan 15 2020, 7:35 AM

Hello Rainer,

could we sed this to index_type instead and address the _t suffix once and for all as you suggest in your description of the problem?
I am not attached to _t and I see the downside now.

Sure, done now. Tested as before.

However, while all mlir tests PASS on Solaris/amd64, there are some failures on Solaris/sparcv9 (which existed before, the patch is just meant to address the compile failure).

MLIR-Unit :: TableGen/./MLIRTableGenTests/StructsGenTest.GetElements

MLIR :: AffineOps/ops.mlir
MLIR :: Conversion/GPUToSPIRV/simple.mlir
MLIR :: Conversion/LoopsToGPU/imperfect_linalg.mlir
MLIR :: Conversion/StandardToLLVM/convert-to-llvmir.mlir
MLIR :: Conversion/StandardToSPIRV/legalization.mlir
MLIR :: Conversion/StandardToSPIRV/std-to-spirv.mlir
MLIR :: Conversion/StandardToSPIRV/subview-to-spirv.mlir
MLIR :: Conversion/VectorToLoops/vector-to-loops.mlir
MLIR :: Dialect/FxpMathOps/lower-uniform-casts.mlir
MLIR :: Dialect/FxpMathOps/lower-uniform-real-math-addew.mlir
MLIR :: Dialect/FxpMathOps/lower-uniform-real-math-mulew.mlir
MLIR :: Dialect/LLVMIR/roundtrip.mlir
MLIR :: Dialect/Linalg/fusion-2-level.mlir
MLIR :: Dialect/Linalg/fusion.mlir
MLIR :: Dialect/Linalg/promote.mlir
MLIR :: Dialect/Linalg/tile.mlir
MLIR :: Dialect/Linalg/tile_conv.mlir
MLIR :: Dialect/Linalg/tile_indexed_generic.mlir
MLIR :: Dialect/Linalg/transform-patterns.mlir
MLIR :: Dialect/QuantOps/canonicalize.mlir
MLIR :: Dialect/QuantOps/convert-const.mlir
MLIR :: Dialect/SPIRV/Serialization/constant.mlir
MLIR :: Dialect/SPIRV/Transforms/abi-load-store.mlir
MLIR :: Dialect/SPIRV/Transforms/abi-simple.mlir
MLIR :: Dialect/SPIRV/canonicalize.mlir
MLIR :: Dialect/SPIRV/structure-ops.mlir
MLIR :: Dialect/SPIRV/target-and-abi.mlir
MLIR :: Dialect/VectorOps/invalid.mlir
MLIR :: Dialect/VectorOps/ops.mlir
MLIR :: IR/attribute.mlir
MLIR :: IR/core-ops.mlir
MLIR :: IR/invalid-ops.mlir
MLIR :: IR/parser.mlir
MLIR :: IR/pretty-attributes.mlir
MLIR :: IR/traits.mlir
MLIR :: Quantizer/matmul.mlir
MLIR :: Target/llvmir.mlir
MLIR :: Transforms/Vectorize/vectorize_1d.mlir
MLIR :: Transforms/Vectorize/vectorize_2d.mlir
MLIR :: Transforms/canonicalize.mlir
MLIR :: Transforms/constant-fold.mlir
MLIR :: mlir-cpu-runner/linalg_integration_test.mlir
MLIR :: mlir-cpu-runner/simple.mlir
MLIR :: mlir-cpu-runner/unranked_memref.mlir
MLIR :: mlir-cpu-runner/utils.mlir
MLIR :: mlir-tblgen/pattern.mlir

I haven't even started looking what might be wrong here (endianess issue?).

Unit tests: pass. 61892 tests passed, 0 failed and 782 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

nicolasvasilache accepted this revision.Jan 15 2020, 7:59 AM

understood, thank you!

This revision is now accepted and ready to land.Jan 15 2020, 7:59 AM
ro updated this revision to Diff 238430.Jan 16 2020, 1:40 AM

Turns out I forgot to rerun clang-format-diff.py after renaming mlir_index_t to
index_type. Fixed now. Also updated mlir/docs/EDSC.md to match.

Unit tests: pass. 61909 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

This revision was automatically updated to reflect the committed changes.