assigned to @tstellar

Sorry, I meant to respont to this long ago.

If you only really care about 64 vs 32 bit multiply, I think this can
be fixed without a target hook, because it will be generally good for
any architecture.

In isSimpleIVUser, just check if the SCEV expression is a multiply with more than one non-constant operand.

Just run LLVM test suite on x86 to verify no significant regressions.

I hope that works...

Hi Andrew,

I think checking if the SCEV is a multiply would work for my cases, and seems to work for Tom's example. It can be a quick work around.

However, the general problem behind the scene still remains in the long term. Just think about a slight variation of my example:

attribute((global)) void foo(int n, int *output) {
for (int i = 0; i < n; i += 3) {
output[i + 2] = i + 4;
}
}

"i" is not involved in any multiply, but widening it to i64 is still a bad idea for the NVPTX backend. We may still want the target-hook approach in a long term.

Jingyue

If your target never wants to widen induction variables, then a target hook makes perfect sense.

Checking for multiply by non-constant makes sense in general.

They're both good fixes.

Agreed. Are you sending out the patch soon? Sounds like it's ready already. Thanks for working on this!

No, I don't have a patch. I think either fix would be easy though.

If your target never wants to widen induction variables, then a target hook
makes perfect sense.

I would prefer adding this target hook, because I don't think we ever want to widen the induction variable for R600.

I'm curious though, in what scenarios is it beneficial to widen induction variables?

Checking for multiply by non-constant makes sense in general.

They're both good fixes.

Hi Tom,

r221799 fixed the issue on the NVPTX side by having IndVarSimplify check the cost of the widen operation. Are you going to take it over and implement the getArithmeticInstrCost interface in AMDGPUTargetTransformInfo? I would be happy to do that but I don't have specific knowledge on the cost model of AMDGPU.

Thanks,
Jingyue

Should be fixed by r264376