In D126402#3538078, @wallace wrote:

thank you!

I'm surprised the compiler didn't do this automatically

In D125764#3521872, @nikic wrote:

Thanks for the explanation, I get it now. It would probably be better to enforce this directly in tablegen, but this seems like a reasonable starting point.

In D125764#3521604, @nikic wrote:

In D125764#3519427, @thopre wrote:

In D125764#3519318, @nikic wrote:

Could you please share an example of a problematic intrinsic signature?

def int_colossus_ststep :
  Intrinsic<
    [LLVMPointerType<llvm_any_ty>],
    [llvm_any_ty,
     LLVMPointerType<LLVMMatchType<0>>,
     llvm_i32_ty],
    [IntrWriteMem, IntrArgMemOnly]>;

Okay, I don't think I really understand the issue you're fixing then. This kind of intrinsic declaration should work with opaque pointers. The LLVMMatchType<0> predicate inside LLVMPointerType<> should just be ignored, which is what the current code is doing (right?)

It looks like we also have some in-tree intrinsics that are using this pattern, such as int_riscv_vlm.

In D125764#3519318, @nikic wrote:

Could you please share an example of a problematic intrinsic signature?

Be more conservative in what we reject

Exclude match arguments

Genuine test failure on CodeGenOpenCL/builtins-fp-atomics-gfx1030.cl

Ping?

LGTM with the suggested codestyle changes

• Rebase
• remove changes to ignoreDependence, it should be done in a separate commit

• Replace isArtificial check by isBoundaryNode
• Remove call to ignoreDependence in computePath for predecessors
• remove no longer needed changes to PowerPC tests

Adding @jsji to review the changes to PowerPC testcases. I've checked the MIR after the pipeliner for sms-phi-3.ll and the loop still gets pipelined (which makes sense since the modified code executes after the decision to pipeline is made) but the code gets simplified by later pass. Are you happy with the change?

Adjust PowerPC testcases

LGTM but I'd prefer someone else to approve.

LGTM with the following description change: very subtle bug horrible bug -> very subtle *but* horrible bug

In D123227#3434107, @jroelofs wrote:

Seems I don't have permissions on the repo to commit this.

In D123227#3433836, @jroelofs wrote:

This showed up in one of our buildbots:

++ lnt check-no-errors lnt-submission.json
lib/python3.8/site-packages/pkg_resources/__init__.py:122: PkgResourcesDeprecationWarning: LNT 0.4.2.dev0 is an invalid version and will not be supported in a future release
  warnings.warn(
++ echo @@@@@@@
@@@@@@@

Is the change about handling cases where the project name is something that would not be valid for parse_version? Could you detail the commit message a little bit to explain what is currently not compliant?

Note: there is still a few reference to mlir::FuncOp in the tree: most notably in the toy example:

Rebase

In D69056#3380914, @cmatthews wrote:

I think this would be safe to land now. Are you still interested in landing it?

In D117445#3249325, @Amir wrote:

In D117445#3247857, @thopre wrote:

What's the reason for not using CLANG instead of BOLT_CLANG_EXE?

I'm a bit worried about an interference with other project tests (ie when bolt and clang are enabled and tests for both are invoked). But I agree it's unlikely to be an issue in practice.

What's the reason for not using CLANG instead of BOLT_CLANG_EXE?

Could you add a new testcase somewhere in llvm/test/FileCheck? Thanks!

LGTM, thanks!

LGTM

So this is to be compatible with sphinx 2.0? LGTM

LGTM

LGTM

On what system isn't the SIGBUS caught?

LGTM otherwise

In D60392#3184688, @jhenderson wrote:

Regarding the CHECK-ASSERT suggestion, do we need it? I was under the impression one of the earlier patches added a syntax like [[#VAR2, VAR1 + 3:]] or something to that effect, which achieves the same thing, right? Maybe I'm missing something though.

Hi Joel,

Adding Tamar to get an Arm person to review this.

LGTM, thanks!

In D112268#3138877, @craig.topper wrote:

In D112268#3138874, @thopre wrote:

In D112268#3138471, @craig.topper wrote:

In D112268#3137708, @thopre wrote:

Hi @craig.topper ,

This caused a regression for us for i16 cttz with is_undef_zero true because cttz is expanded to a sequence using ctpop which loses the fact that any output is ok for the 0 case.

t17: i16 = sub t9, Constant:i16<1>
t19: i16 = xor t9, Constant:i16<-1>
t20: i16 = and t19, t17
t21: i16 = ctpop t20

So when this gets promoted, a mask is inserted to not have the result of ctpop change when t9 is 0.

t15: i32 = extract_vector_elt t2, Constant:i32<0>
t25: i32 = xor t15, Constant:i32<-1>
t23: i32 = sub t15, Constant:i32<1>
t26: i32 = and t25, t23
t28: i32 = and t26, Constant:i32<65535>
t29: i32 = ctpop t28

Prior to this patch, we'd get:

t19: i32 = sub t15, Constant:i32<1>
t21: i32 = xor t15, Constant:i32<-1>
t22: i32 = and t21, t19
t23: i32 = ctpop t22

I thought about inserting some llvm.assume saying the value is non zero when expanding but those work at IR level. Any pointer on how to solve this? Best regards.

I take it i32 ctpop is legal for your target or the i16 ctpop would have gotten expanded too?

Correct.

I just pushed d78fdf111dda26307e88d16f5f5d1411f3bd7e61 can you let me know if that fixes the issue?

In D112268#3138471, @craig.topper wrote:

In D112268#3137708, @thopre wrote:

Hi @craig.topper ,

This caused a regression for us for i16 cttz with is_undef_zero true because cttz is expanded to a sequence using ctpop which loses the fact that any output is ok for the 0 case.

t17: i16 = sub t9, Constant:i16<1>
t19: i16 = xor t9, Constant:i16<-1>
t20: i16 = and t19, t17
t21: i16 = ctpop t20

So when this gets promoted, a mask is inserted to not have the result of ctpop change when t9 is 0.

t15: i32 = extract_vector_elt t2, Constant:i32<0>
t25: i32 = xor t15, Constant:i32<-1>
t23: i32 = sub t15, Constant:i32<1>
t26: i32 = and t25, t23
t28: i32 = and t26, Constant:i32<65535>
t29: i32 = ctpop t28

Prior to this patch, we'd get:

t19: i32 = sub t15, Constant:i32<1>
t21: i32 = xor t15, Constant:i32<-1>
t22: i32 = and t21, t19
t23: i32 = ctpop t22

I thought about inserting some llvm.assume saying the value is non zero when expanding but those work at IR level. Any pointer on how to solve this? Best regards.

I take it i32 ctpop is legal for your target or the i16 ctpop would have gotten expanded too?

LGTM

This caused a regression for us for i16 cttz with is_undef_zero true because cttz is expanded to a sequence using ctpop which loses the fact that any output is ok for the 0 case.

LGTM

LGTM

In D113470#3118020, @tnfchris wrote:

In D113470#3118004, @thopre wrote:

In D113470#3117975, @tnfchris wrote:

Yes this works fine and doesn't interfere with our approach.

We took a different route here in that we COPY the lnt.cfg from outside the container into it to /etc/lnt-new.cfg and then restore it after the create command
but before the exec.

We do this since there are other settings we want to preserve, such as mail server settings etc.

Sounds like create-config should be modified to honor an existing config file so that you could just use a ADD in your Dockerfile

Yeah, I think that's the best long term approach, but doesn't need to block your change here though.

In D113470#3117975, @tnfchris wrote:

Yes this works fine and doesn't interfere with our approach.

We took a different route here in that we COPY the lnt.cfg from outside the container into it to /etc/lnt-new.cfg and then restore it after the create command
but before the exec.

We do this since there are other settings we want to preserve, such as mail server settings etc.

LGTM

In D111289#3093293, @kpdev42 wrote:

Updated patch after landing https://reviews.llvm.org/D112607 and https://reviews.llvm.org/D112525

P.S.: dear reviewers, please do not commit this patch without mentioning me as an author, because it may raise some legal questions from my company )

In D112607#3092898, @kpdev42 wrote:

In D112607#3092853, @thopre wrote:

LGTM with the proposed changes. I'll land it for you with them.

Please check, thank you

LGTM with the proposed changes. I'll land it for you with them.