Page MenuHomePhabricator

IR: Add immarg attribute
ClosedPublic

Authored by arsenm on Feb 6 2019, 8:48 AM.

Details

Summary

This indicates an intrinsic parameter is required to be a constant,
and should not be replaced with a non-constant value.

Add the attribute to all AMDGPU and generic intrinsics that comments
indicate it should apply to. I scanned other target intrinsics, but I
don't see any obvious comments indicating which arguments are intended
to be only immediates.

This breaks one questionable testcase for the autoupgrade. I'm unclear
on whether the autoupgrade is supposed to really handle declarations
which were never valid. The verifier fails because the attributes now refer
to a parameter past the end of the argument list.

Diff Detail

Event Timeline

arsenm created this revision.Feb 6 2019, 8:48 AM
arsenm updated this revision to Diff 185573.Feb 6 2019, 9:14 AM

Remove leftover change

craig.topper added inline comments.
docs/LangRef.rst
1180

Should this say immarg?

lib/IR/Verifier.cpp
2848

immarg?

arsenm updated this revision to Diff 185588.Feb 6 2019, 10:37 AM

Fix name

Looks reasonable to me. Maybe give a little more time for people to give feedback? Though this has already been on llvm-dev without opposition...

arsenm added a comment.EditedFeb 8 2019, 5:51 AM

@nhaehnle can you look at the InstCombineSimplifyDemanded change? I was a bit confused by the assert you added

@nhaehnle can you look at the InstCombineSimplifyDemanded change? I was a bit confused by the assert you added

The background of this is that if TFE/LWE is enabled, the SimplifyDemanded logic won't work as-is; but it should also never be hit, because intrinsic calls with TFE/LWE should have a struct return type (e.g. {v4f32,i32}) and the SimplifyDemanded logic doesn't support looking through that. The assert double-checks that.

In hindsight, it would be possible for somebody to manually create malformed IR which calls image intrinsics with TFE/LWE enabled but with a vector return type. That would be an error, and one could argue that the code should produce an error instead of an assert. It would require a broken frontend or manually written IR, though.

arsenm added a comment.Feb 8 2019, 7:51 AM

@nhaehnle can you look at the InstCombineSimplifyDemanded change? I was a bit confused by the assert you added

The background of this is that if TFE/LWE is enabled, the SimplifyDemanded logic won't work as-is; but it should also never be hit, because intrinsic calls with TFE/LWE should have a struct return type (e.g. {v4f32,i32}) and the SimplifyDemanded logic doesn't support looking through that. The assert double-checks that.

In hindsight, it would be possible for somebody to manually create malformed IR which calls image intrinsics with TFE/LWE enabled but with a vector return type. That would be an error, and one could argue that the code should produce an error instead of an assert. It would require a broken frontend or manually written IR, though.

I would expect TFE to be turned on by the usage of the struct type. We should probably add a custom verifier check for this

@nhaehnle can you look at the InstCombineSimplifyDemanded change? I was a bit confused by the assert you added

The background of this is that if TFE/LWE is enabled, the SimplifyDemanded logic won't work as-is; but it should also never be hit, because intrinsic calls with TFE/LWE should have a struct return type (e.g. {v4f32,i32}) and the SimplifyDemanded logic doesn't support looking through that. The assert double-checks that.

In hindsight, it would be possible for somebody to manually create malformed IR which calls image intrinsics with TFE/LWE enabled but with a vector return type. That would be an error, and one could argue that the code should produce an error instead of an assert. It would require a broken frontend or manually written IR, though.

I would expect TFE to be turned on by the usage of the struct type. We should probably add a custom verifier check for this

Yes, but there are different modes to TFE/LWE, so we'd still need the argument anyway.

arsenm updated this revision to Diff 186848.Feb 14 2019, 7:58 AM

Add to a few more intrinsics that the verifier checked. The call site restriction may need to be relaxed since the state point intrinsics seem to be concerned with an immediate argument in the variadic argument list

craig.topper added inline comments.Feb 14 2019, 11:15 PM
lib/IR/Verifier.cpp
4593

This check can be removed now

arsenm updated this revision to Diff 187037.Feb 15 2019, 10:29 AM

Handle a few more intrinsics, drop more custom checks

pcc added a subscriber: pcc.Feb 15 2019, 10:38 AM
pcc added inline comments.
include/llvm/IR/Intrinsics.td
1079

The third argument of this intrinsic needs to be an immediate.

arsenm updated this revision to Diff 187266.Feb 18 2019, 12:08 PM
arsenm marked an inline comment as done.

Add release note

Changes here seem ok, but let me confirm: do we expect any changes/harm for targets that are not in the stack of dependent patches? Either way, reply to the llvm-dev thread to remind those targets and out-of-tree targets that this change has been committed (once it lands)?

Changes here seem ok, but let me confirm: do we expect any changes/harm for targets that are not in the stack of dependent patches? Either way, reply to the llvm-dev thread to remind those targets and out-of-tree targets that this change has been committed (once it lands)?

Yes, but only after D58233. Until D58233, these aren't mandatory. After that, these need to be accurate or else passes may incorrectly replace arguments if these aren't accurate

spatel accepted this revision.Mar 12 2019, 12:14 PM

LGTM

This revision is now accepted and ready to land.Mar 12 2019, 12:14 PM
arsenm closed this revision.Mar 12 2019, 2:02 PM

r355981