[PATCH] D81045: [MLIR] Modify HasParent trait to allow one of several op's as a parent

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar added inline comments.



Comment at: llvm/include/llvm/Support/Casting.h:147
+template 
+LLVM_NODISCARD inline typename std::enable_if::type
+isa(const Y &Val) {

I'm worried that folks may not notice this change based on the change 
description (e.g., the change sounds MLIR specific), could you make this a 
separate change? (just focused on isa changes)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81045



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81422: Change filecheck default to dump input on failure

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar accepted this revision.
jpienaar added a comment.

I think this is a good default given it provides very useful info in failure 
case. For cases where folks expect it to fail + don't want it logged, then 
setting never (if that is a concern) seems better behavior IMHO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123836: [Driver] Move Lanai IAS enabling to Generic_GCC::IsIntegratedAssemblerDefault, NFC

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar accepted this revision.
jpienaar added a comment.

Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123836

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar added a comment.

> (1) the operands defined by non-constant-like ops come first, followed by (2) 
> block arguments, and these are followed by (3) the operands defined by 
> constant-like ops.

I would have thought block-arguments would come first as we don't know their 
values, while non-constant-like ops could be folded at some point and then 
become constant-like. Meaning, they seem closer to constant than block 
arguments.

+1 to Mehdi's question about just stable sorting based on based on 4 criteria 
(3 buckets + ordering within (1)) and then we should be able to avoid all the 
string mangling too as Jeff asked about.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82860: Port ObjCMTAction to new option parsing system

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar added a comment.

This seems to breaking clang-5 builds:

utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptParserEmitter.cpp.o: In function 
`llvm::EmitOptParser(llvm::RecordKeeper&, llvm::raw_ostream&)':
/var/lib/buildkite-agent/builds/buildkite-69fdf6c495-wt2bd-1/mlir/mlir-core/llvm/utils/TableGen/OptParserEmitter.cpp:(.text._ZN4llvm13EmitOptParserERNS_12RecordKeeperERNS_11raw_ostreamE+0x148a):
 undefined reference to `MarshallingInfo::MacroName'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

(https://buildkite.com/mlir/mlir-core/builds/9273#ae602c1f-8c21-4ac6-90bb-99c9a3ae473e)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82860

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91410: [llvm][clang][mlir] Add checks for the return values from Target::createXXX to prevent protential null deref

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar accepted this revision.
jpienaar added a comment.

LG MLIR changes


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

https://reviews.llvm.org/D91410

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140896: [WIP] Move from llvm::makeArrayRef to ArrayRef deduction guides

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar added inline comments.



Comment at: llvm/include/llvm/ADT/ArrayRef.h:502
+  /// Deduction guide to construct an ArrayRef from a C array.
+  template  ArrayRef(const T (&Arr)[N]) -> ArrayRef;
 

mehdi_amini wrote:
> Can we keep the makeArrayRef functions for now and mark them deprecated?
+1 that would also allow for this change to broken up so that the deduction 
guides land first and then the updates (potentially even 3 hops, 1) add guides, 
2) update, 3) mark deprecated - 1 & 2 could be combined but it'll greatly 
simplify review/enable sharding it)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140896

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128049: [mlir] move SCF headers to SCF/{IR,Transforms} respectively

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar accepted this revision.
jpienaar added a comment.
This revision is now accepted and ready to land.

More consistency is good and update for downstream users mechanical, so LGTM




Comment at: mlir/include/mlir/Dialect/SCF/Transforms/Transforms.h:13
 
-#ifndef MLIR_DIALECT_SCF_TRANSFORMS_H_
-#define MLIR_DIALECT_SCF_TRANSFORMS_H_
+#ifndef MLIR_DIALECT_SCF_TRANSFORMS_TRANSFORMS_H_
+#define MLIR_DIALECT_SCF_TRANSFORMS_TRANSFORMS_H_

Transforms transforms feels a bit strange, for many others I believe this would 
have been passes file (which is also not that accurate, patterns and passes 
would be more, but most others it is just passes and convenient shorthand). 
Keeping the move mostly mechanical is good though 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128049

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76262: [NFC] Add UsedDeclVisitor

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar added a comment.

This does not appear to be NFC: 
 git checkout 704cd4d5d0754904361823588f203369c309deca 
 ; ninja 
check-mlir passes
 git checkout 08ab8c9af4dd27cb306b449edc9a9c50ed11194a 
 ; ninja 
check-mlir fails with:

  0.  Program arguments: Compiles/build_clang/bin/clang++ -DBUILD_EXAMPLES 
-DGTEST_HAS_RTTI=0 -DMLIR_CUDA_CONVERSIONS_ENABLED=0 -D_DEBUG -D_GNU_SOURCE 
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
-Itools/mlir/lib/Transforms -Illvm-project/mlir/lib/Transforms -Iinclude 
-Illvm-project/llvm/include -Illvm-project/mlir/include -Itools/mlir/include 
-fPIC -fvisibility-inl
  ines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall 
-Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual 
-Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough 
-Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor 
-Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color 
-ffunction-sections -fdata-sections -O3 -fno-exceptions -fno-rtti -UNDEBUG 
-std=c++14 -MD -MT 
tools/mlir/lib/Transforms/CMakeFiles/MLIRTransforms.dir/AffineDataCopyGeneration.cpp.o
 -MF 
tools/mlir/lib/Transforms/CMakeFiles/MLIRTransforms.dir/AffineDataCopyGeneration.cpp.o.d
 -o 
tools/mlir/lib/Transforms/CMakeFiles/MLIRTransforms.dir/AffineDataCopyGeneration.cpp.o
 -c llvm-project/mlir/lib/Transforms/AffineDataCopyGeneration.cpp
  1.   parser at end of file
  2.  Per-file LLVM IR generation
  3.  
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/unique_ptr.h:848:5:
 Generating code for declaration 'std::make_unique'
  
build_clang/bin/clang++(_ZN4llvm3sys15PrintStackTraceERNS_11raw_ostreamE+0x1a)[0x55796bf8436a]
  
build_clang/bin/clang++(_ZN4llvm3sys17RunSignalHandlersEv+0x34)[0x55796bf82204]
  build_clang/bin/clang++(_ZN4llvm3sys15CleanupOnSignalEm+0xf8)[0x55796bf82708]
  build_clang/bin/clang++(+0x1a58a08)[0x55796bf07a08]
  /lib/x86_64-linux-gnu/libpthread.so.0(+0x13520)[0x7f1b727f7520]
  /lib/x86_64-linux-gnu/libc.so.6(abort+0x121)[0x7f1b722bd535]
  /lib/x86_64-linux-gnu/libc.so.6(+0x2540f)[0x7f1b722bd40f]
  /lib/x86_64-linux-gnu/libc.so.6(+0x32b92)[0x7f1b722cab92]
  
build_clang/bin/clang++(_ZN5clang7CodeGen15CodeGenFunction17EmitDeclRefLValueEPKNS_11DeclRefExprE+0x358)[0x55796c4cb138]
  
build_clang/bin/clang++(_ZN5clang7CodeGen15CodeGenFunction10EmitLValueEPKNS_4ExprE+0x2f9)[0x55796c4ca6d9]
  
build_clang/bin/clang++(_ZN5clang7CodeGen15CodeGenFunction14EmitCastLValueEPKNS_8CastExprE+0x103)[0x55796c4d4033]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar added a comment.

This seems to result in triggering clang/lib/CodeGen/CGExpr.cpp:2626 when 
compiling mlir/lib/Transforms/AffineDataCopyGeneration.cpp with clang build 
with assertions on (clean build at e8e078c 
 just 
before this change, broken at this, assert triggering at build fix commit).

https://buildkite.com/mlir/mlir-core/builds/2792#a54fb239-718b-4f0b-a309-f83e46ceb252


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70172



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144911: adding bf16 support to NVPTX

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar added a comment.

In D144911#4347029 , @kushanam wrote:

> In D144911#4340097 , @jpienaar 
> wrote:
>
>> Thanks Artem for explaining,
>
> I think somehow, Phabricator pushes my changes to 
> https://reviews.llvm.org/D149976 instead. The changes are in the patch for 
> now, Is it possible to continue the review there instead?

This seems merged again, is that correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144911

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144911: adding bf16 support to NVPTX

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar added inline comments.



Comment at: llvm/lib/Target/NVPTX/NVPTXInstrInfo.td:3352
 
-  // Coalesce two bf16 registers into bf16x2
-  def BuildBF16x2 : NVPTXInst<(outs BFloat16x2Regs:$dst),
- (ins BFloat16Regs:$a, BFloat16Regs:$b),
- "mov.b32 \t$dst, {{$a, $b}};",
- [(set (v2bf16 BFloat16x2Regs:$dst),
-   (build_vector (bf16 BFloat16Regs:$a), (bf16 
BFloat16Regs:$b)))]>;
-
-  // Directly initializing underlying the b32 register is one less SASS
-  // instruction than than vector-packing move.
-  def BuildBF16x2i : NVPTXInst<(outs BFloat16x2Regs:$dst), (ins i32imm:$src),
-  "mov.b32 \t$dst, $src;",
-  []>;
-
-  // Split f16x2 into two f16 registers.
-  def SplitBF16x2  : NVPTXInst<(outs BFloat16Regs:$lo, BFloat16Regs:$hi),
-  (ins BFloat16x2Regs:$src),
-  "mov.b32 \t{{$lo, $hi}}, $src;",
-  []>;
-  // Split an i32 into two f16
-  def SplitI32toBF16x2  : NVPTXInst<(outs BFloat16Regs:$lo, BFloat16Regs:$hi),
-   (ins Int32Regs:$src),
-   "mov.b32 \t{{$lo, $hi}}, $src;",
-   []>;
+  // // Coalesce two bf16 registers into bf16x2
+  // def BuildBF16x2 : NVPTXInst<(outs BFloat16x2Regs:$dst),

Could these commented parts be dropped?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144911

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138668: Correct typos

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar added a comment.

Do you need help in landing change? Or waiting for all reviewers?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138668

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138668: Correct typos

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar closed this revision.
jpienaar added a comment.

Landed in 
https://github.com/llvm/llvm-project/commit/a9f9f3dff474b7bdb19129eaf625d3ef0084a975


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138668

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116488: Add a misc-unused-parameters.CommentOutUnusedParameters to clang-tidy

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst:25
+
+  void a(int ) { /*some code that doesn't use `i`*/ }
+

OOC why the extra space after the type?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116488: Add a misc-unused-parameters.CommentOutUnusedParameters to clang-tidy

[Jacques Pienaar via Phabricator via cfe-commits]

jpienaar accepted this revision.
jpienaar added a comment.
This revision is now accepted and ready to land.

Looks reasonable as an option/this is another way to make it explicit that 
param is unused.




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst:48
+
+.. option:: misc-unused-parameters.CommentOutUnusedParameters
+

Could we flip this? ElideUnusedParameterNames with default value false: eliding 
drops local information and I'd rather that be explicit in the name. CommentOut 
is still the default with the current naming, but it feels more optional for 
some reason to me. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits