[PATCH] D90231: [GVN] Don't replace argument to @llvm.is.constant.*()

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added a comment.

In D90231#2358870 <https://reviews.llvm.org/D90231#2358870>, @joerg wrote:

> I completely agree with Roman that this patch is very undesirable since it has a high chance of breaking other reasonable uses of the intrinsic.

Would it help if the replaceXXXUsesWith() only excluded the intrinsic from this particular GVN optimization, and not all the time?

> In fact, I would say that the observed behavior here is perfectly reasonable under the definition of @lllvm.is.constant. The original code for LLVM at least should be just `__builtin_constant_p(b) && b > -129 && b < 128)`.

The example is reduced but still represents code in the Linux kernel compilable by GCC, but not by clang. Here is a bigger picture (still reduced, but not as much:

  static inline __attribute__((__gnu_inline__)) __attribute__((__unused__))
  __attribute__((__no_instrument_function__))
  __attribute__((__always_inline__)) void
  __atomic_add_const(int val, int *ptr)
  {
  	asm volatile("asi"
  		     "	%[ptr],%[val]\n"
  		     "\n"
  		     : [ptr] "+Q"(*ptr)
  		     : [val] "i"(val)
  		     : "cc", "memory");
  }
  
  static inline __attribute__((__gnu_inline__)) __attribute__((__unused__))
  __attribute__((__no_instrument_function__)) int
  __atomic_add(int val, int *ptr)
  {
  	int old;
  	asm volatile("laa"
  		     "	%[old],%[val],%[ptr]\n"
  		     "\n"
  		     : [old] "=d"(old), [ptr] "+Q"(*ptr)
  		     : [val] "d"(val)
  		     : "cc", "memory");
  	return old;
  }
  
  static inline __attribute__((__gnu_inline__)) __attribute__((__unused__))
  __attribute__((__no_instrument_function__)) void
  atomic_add(int i, atomic_t *v)
  {
  	if ((__builtin_constant_p(
  		     !!((i > -129) && (i < 128) && __builtin_constant_p(i))) ?
  			   (!!((i > -129) && (i < 128) && __builtin_constant_p(i))) :
  			   ({ (!!((i > -129) && (i < 128) && __builtin_constant_p(i))) ? 1 : 0; }))) {
  		__atomic_add_const(i, &v->counter);
  		return;
  	}
  
  	__atomic_add(i, &v->counter);
  }
  
  void tty_buffer_reset(long size);
  void tty_buffer_alloc(atomic_t v, long size)
  {
  	size = ((((size)) + ((255))) & ~((255)));
  
  	tty_buffer_reset(size);
  	atomic_add(size, &v);
  }
  
  #define IB_SET_POST_CREDITS(v)	((v) << 16)
  void rds_ib_advertise_credits(atomic_t v, unsigned int posted)
  {
  	atomic_add(IB_SET_POST_CREDITS(posted), &v);
  }
  
  /*
   * more cases like:
   * atomic64_add((u64)bo->num_pages << PAGE_SHIFT, &rdev->num_bytes_moved);
   */

As this shows,  atomic_add_const() is supposed to be called only when atomic_add() is called with a compile-time constant. Note that __atomic_add_const() (after inlining etc) will not even compile if the argument is not const, since it uses the "i" inline asm constraint. So having a runtime check against 0 is not acceptable - it doesn't compile!

I wonder

- what other use case of __builtin_constant_p() would make this runtime check desirable?
- what other way than disabling this in GVN is there to make the test case compile?


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

https://reviews.llvm.org/D90231